Group chat without an avatar uses the avatar of first person to join

[kittykat]

Steps to reproduce

1. Start a group chat
2. User joins group chat

Outcome

What did you expect?

Group chat without avatar does not have an avatar set

What happened instead?

The avatar of the first user to join is used for the group chat

Internal reference: iOSX-6

Your phone model

No response

Operating system version

No response

Application version

No response

Homeserver

No response

Will you send logs?

No

[pixlwave]

Currently the avatar is computed by the proxy. It is ignoring the isDirect flag which wasn't the exact behaviour we had planned.

matrix-org/sliding-sync#208 (comment)

I have chosen to deliberately ignore changes to the DM flag. In fact, I don't think CalculateAvatar even reads the DM flag. I expect this will be good enough for a first pass.

Thinking about this, now that we have access to the room heroes, we could probably start using those locally with the exact rules we would like and stop computing the avatar on the server.

[Hywan]

Room heros aren't supported yet as far as I know, matrix-org/matrix-rust-sdk#2702. It's under implementation.

[bnjbvr]

Right now, the avatar is computed on the server. The implementation didn't take care of this use case. Here are the possible ways to move forward:

Have the client compute heroes

This would require implementing the concept of heroes in the client's sliding sync. Of course, it would be the most invasive change, since we never had that, and even commented in the code that we'd probably never implement it, since the server does compute the avatars for us. We'd have perfect control over what we display, at the cost of reimplementing it all by ourselves, and requiring that every client that doesn't use the Rust SDK also re-implement it by themselves.

Tweak the server response

I think the simplest change and best way to move forward would be to tweak the server-side rules:

• either a room has a set avatar, in that case return it
• or it hasn't:
  • if there are exactly 2 people in that room, return the other person's avatar
  • if there's 1 (only us) or >2, return no avatar

Does that sound reasonable and easily doable @DMRobertson @kegsay? (If yes, sorry to add things to your todo-list.)

Addendum: can the client try to remove the avatar from the server, based on the number of room members?

Unfortunately no: the client can't distinguish "a room has an avatar that has been explicitly set" from "a room has no avatar and the server returned the avatar of the first person". So if we added a rule to remove avatars if a group conversation with >2 people had one, we could end up in a situation where we'd incorrectly remove an avatar that was explicitly set.

[DMRobertson]

the client can't distinguish "a room has an avatar that has been explicitly set" from "a room has no avatar and the server returned the avatar of the first person".

You could distinguish this by requesting the m.room.avatar state event. But: the the server should be providing useful information in this field, and if it isn't we should fix it.

• either a room has a set avatar, in that case return it

• or it hasn't:

  • if there are exactly 2 people in that room, return the other person's avatar
  • if there's 1 (only us) or >2, return no avatar

This should be exactly the logic the sliding sync proxy is using today: https://github.com/matrix-org/sliding-sync/blob/62d3798955eaf183fc36111b543fe329e570c88e/internal/roomname.go#L264-L275

[bnjbvr]

This should be exactly the logic the sliding sync proxy is using today: https://github.com/matrix-org/sliding-sync/blob/62d3798955eaf183fc36111b543fe329e570c88e/internal/roomname.go#L264-L275

Ah thanks, that's good to know. Then the situation exposed in this issue should not happen.

What I suppose is the error was that someone created a group, invited >2 people, first person joined and their avatar was used as the room avatar, and then somebody joined but the room avatar still was that of the first person. I double-checked and the client should update the avatar correctly in this situation (I'll write a test for that).

I wonder if the server is not sending us an updated avatar because CalculateRoomAvatar uses different conditions from SameRoomAvatar (which is used before calling CalculateRoomAvatar)?

[DMRobertson]

I wonder if the server is not sending us an updated avatar because CalculateRoomAvatar uses different conditions from SameRoomAvatar (which is used before calling CalculateRoomAvatar)?

It's been a while since I wrote this, but from a quick view it looks like the conditions match up.

What I suppose is the error was that someone created a group, invited >2 people, first person joined and their avatar was used as the room avatar, and then somebody joined but the room avatar still was that of the first person.

That's possible!

We do have an E2E testing covering a "group DM" with three users (whatever that means), but it's created in one go by atomically inviting the other two at creation time: https://github.com/matrix-org/sliding-sync/blob/37aa1469a53afecf4226bff7ea11fd7859a79af0/tests-e2e/lists_test.go#L1373-L1377

It should be easy enough to recreate any reproduction steps in this test, if anyone can confirm how to trigger this.

[bnjbvr]

It should be easy enough to recreate any reproduction steps in this test, if anyone can confirm how to trigger this.

Here you go! I wrote an integration test to help reproduce and find where the issue is, and I have a confirmation that it's the proxy server returning the avatar for one of the invitees. Instructions are included in that PR to reproduce it at home.

[bnjbvr]

Sorry, turns out it might be the Rust SDK's fault after all: we only memorize a new room avatar when we see one, and never delete it, which is technically a bug if it's been unset.

Unfortunately, there's a comment above the code doing that, suggesting that the proxy server may not return data that hasn't changed. Ruma deserializes the avatar field from the response as an Option<MxUri>, and so the absence of the field is indistinguishable from the field being set to null. I suspect a removal would happen as setting the field to null, so we'd need to fix Ruma or make sure the server always return the avatar URI or the server returns a sentinel value when it's setting the avatar to none. To be continued...

[bnjbvr]

Sorry again for jumping to conclusions and blaming the proxy server on this; thanks to @DMRobertson and @stefanceriu for the group investigation.

In fact, it's both the proxy and the client misbehaving here (things can't be black or white, can they?). When starting such a group conversation, first the proxy starts with setting the avatar_url to the first user's avatar; but then, a sync or so later, it cancels that by assigning the avatar back to null (likely when it discovers there are more than 2 people in the conversation). I think there might be a small bug first, but it doesn't need fixing in the absolute, since clients ought to update accordingly.

Then, the bug in the client (explained in the previous comment) dismissed the second update, and kept the group avatar set to the first person's avatar. This should be fixed in this PR.

[bnjbvr]

I want to claim fixed by matrix-org/matrix-rust-sdk#2867 as Stefan couldn't reproduce the issue with my patch, so closing. Please reopen if there's still a bug here.

[VolkerJunginger]

Untitled.mp4

The bug is still there. See video.

[stefanceriu]

Alrighty, this time round it does look like it's a backend issue where the room gets an avatar that's never unset (like the previous behavior)

If I am to venture a guess I would say it's not exactly what Doug said at the beginning of this thread: the room only has 2 members, it's considered a dm and this logic kicks in https://github.com/matrix-org/sliding-sync/pull/208/files#diff-591e344e78956e112ee213f5ab8e2dd85ee4d6d327ddb85a06ec23910fac4a4fR257

cc @bnjbvr and epecially @DMRobertson

-- Room creation --

{
  "initial_state": [
    {
      "type": "m.room.encryption",
      "content": {
        "algorithm": "m.megolm.v1.aes-sha2"
      },
      "state_key": ""
    }
  ],
  "invite": [
    "@stefan.ceriu-element02:matrix.org"
  ],
  "name": "Test avatar room",
  "preset": "private_chat",
  "topic": ""
}

{
  "room_id": "!XbtYgHzlkCeXPpDAfV:matrix.org"
}

-- SS Responses --

• First without an avatar

{
  "lists": {
    "all_rooms": {
      "count": 228
    },
    "invites": {
      "count": 0
    },
    "visible_rooms": {
      "count": 228
    }
  },
  "rooms": {
    "!XbtYgHzlkCeXPpDAfV:matrix.org": {
      "name": "Test avatar room",
      "timeline": [
        {
          "content": {
            "ban": 50,
            "events": {
              "m.room.avatar": 50,
              "m.room.canonical_alias": 50,
              "m.room.encryption": 100,
              "m.room.history_visibility": 100,
              "m.room.name": 50,
              "m.room.power_levels": 100,
              "m.room.server_acl": 100,
              "m.room.tombstone": 100
            },
            "events_default": 0,
            "historical": 100,
            "invite": 0,
            "kick": 50,
            "redact": 50,
            "state_default": 50,
            "users": {
              "@stefan.ceriu:matrix.org": 100
            },
            "users_default": 0
          },
          "origin_server_ts": 1701764974796,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.power_levels",
          "unsigned": {
            "age": 595
          },
          "event_id": "$dKNcQTXaw6sUJj7lpuVnvMc24EXYPXZrlrqTQdRa9Cw"
        },
        {
          "content": {
            "join_rule": "invite"
          },
          "origin_server_ts": 1701764974818,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.join_rules",
          "unsigned": {
            "age": 573
          },
          "event_id": "$G4imzQDfCfZudCnn6KBMC5ByUq_LG_u5a70i6MEPRDg"
        },
        {
          "content": {
            "history_visibility": "shared"
          },
          "origin_server_ts": 1701764974819,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.history_visibility",
          "unsigned": {
            "age": 572
          },
          "event_id": "$E_KWRYBKDSkwBoKx1ZQtOPh-0Xv24BbIyTkmXtsyGRs"
        },
        {
          "content": {
            "guest_access": "can_join"
          },
          "origin_server_ts": 1701764974819,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.guest_access",
          "unsigned": {
            "age": 572
          },
          "event_id": "$58a6SQji3DJZViOR5v0u3XJv2GPLqNcMyAE5_CDZJHE"
        },
        {
          "content": {
            "algorithm": "m.megolm.v1.aes-sha2"
          },
          "origin_server_ts": 1701764974820,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.encryption",
          "unsigned": {
            "age": 571
          },
          "event_id": "$ADAhk8Cp9dD30nfRPq2YgUzGYxmynk1XqS3s-obspTo"
        },
        {
          "content": {
            "name": "Test avatar room"
          },
          "origin_server_ts": 1701764974821,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.name",
          "unsigned": {
            "age": 570
          },
          "event_id": "$7yDjljYMySmXmRKrRYZhn6n6s3LqTpPA3vy-2eH-sVo"
        },
        {
          "content": {
            "topic": ""
          },
          "origin_server_ts": 1701764974822,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "",
          "type": "m.room.topic",
          "unsigned": {
            "age": 569
          },
          "event_id": "$OL6gj4h8ie3S8PWHJdH3H2DMU-HBd-pWcYlE2rt-NAQ"
        }
      ],
      "notification_count": 0,
      "highlight_count": 0,
      "prev_batch": "s4518350522_757284974_4177703_2506169418_2651547040_5060953_1132588854_9157398177_0_208127",
      "num_live": 7
    }
  },
  "extensions": {},
  "pos": "14"
}

• Then with an avatar

{
  "lists": {
    "all_rooms": {
      "count": 228
    },
    "invites": {
      "count": 0
    },
    "visible_rooms": {
      "count": 228
    }
  },
  "rooms": {
    "!XbtYgHzlkCeXPpDAfV:matrix.org": {
      "name": "Test avatar room",
      "avatar": "mxc://matrix.org/JglrBpuwXwFYxcprgAdlSeks",
      "timeline": [
        {
          "content": {
            "avatar_url": "mxc://matrix.org/JglrBpuwXwFYxcprgAdlSeks",
            "displayname": "Stefan Ceriu Element 02",
            "membership": "invite"
          },
          "origin_server_ts": 1701764975218,
          "sender": "@stefan.ceriu:matrix.org",
          "state_key": "@stefan.ceriu-element02:matrix.org",
          "type": "m.room.member",
          "unsigned": {
            "age": 289
          },
          "event_id": "$KrYRcT50KbOEQmwGt4FHeL3qEQt6O5eSJHwqUx06s7Q"
        }
      ],
      "notification_count": 0,
      "highlight_count": 0,
      "invited_count": 1,
      "prev_batch": "s4518350542_757284974_4177718_2506169421_2651547048_5060953_1132588858_9157398178_0_208127",
      "num_live": 1
    }
  },
  "extensions": {},
  "pos": "15"
}

-- Subsequent SS connections --

• Response room always contains avatar

https://slidingsync.lab.matrix.org/_matrix/client/unstable/org.matrix.msc3575/sync?timeout=30000

"!XbtYgHzlkCeXPpDAfV:matrix.org": {
      "name": "Test avatar room",
      "avatar": "mxc://matrix.org/JglrBpuwXwFYxcprgAdlSeks",

[DMRobertson]

the room only has 2 members, it's considered a dm

Right---I could have sworn that's the behaviour we wanted when I was implementing this.

I can change the logic to make the avatar field only use the other member's avatar if the room is a DM according to account data. But due to recent circumstances, I'll have to get it done before Friday 8th Dec.

[manuroe]

Should be fixed by matrix-org/sliding-sync#394.

[stefanceriu]

Should be fixed by matrix-org/sliding-sync#394.

Oh nice 👏

[manuroe]

The last release of the sliding sync proxy (https://github.com/matrix-org/sliding-sync/releases/tag/v0.99.15) contains the fix and it is now live for matrix.org users.
@VolkerJunginger can you test it and close this ticket when you are happy?