Changing nickname resets highlight_count

[germain-gg]

Description

Changing your nickname resets highlight_count and tranfer its value to notification_count

View original ticket on element-hq/element-web#15860

I originally investigated this issue across matrix-react-sdk and matrix-js-sdk and this led me to the sync endpoint.
I see that data.rooms.join.{ROOM-ID}.unread_notifications returns an unexpected value.
The property highlight_count should be x but instead gets reset to 0 and the notification_count was 0 but is set to x

// state of "data.rooms.join.{ROOM-ID}.unread_notifications" before changing nickname
{
  "highlight_count": 3,
  "notification_count": 1
}

// change nickname
// state of "data.rooms.join.{ROOM-ID}.unread_notifications"
{
  "highlight_count": 0,
  "notification_count": 4
}

I captured what was yielded in the network tab of the Chrome DevTools notification_count.har.zip

Steps to reproduce

notification_count.mov

Version information

• Homeserver: matrix.org

[rxl881]

Heads-up that I got bitten by this yesterday in a pretty frustrating way, while changing my display name to indicate that I was away, sick. Having returned I've now lost all of the notification. badges and as I'm in a very large number of rooms I can't find all of the places where I was pinged / mentioned.

@neilisfragile -- This is currently tagged as a minor, but I'd argue that this is seriously impacting (for people that hit it)?

[DMRobertson]

Reproducible with a synapse test case:

class TODOPickANameForTestCase(unittest.HomeserverTestCase):
    servlets = [
        admin.register_servlets_for_client_rest_resource,
        login.register_servlets,
        profile.register_servlets,
        room.register_servlets,
        sync.register_servlets,
    ]

    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
        self.alice = self.register_user("alice", "pass")
        self.alice_tok = self.login("alice", "pass")
        self.bob = self.register_user("bob", "pass")
        self.bob_tok = self.login("bob", "pass")

    def test_highlight_count_after_changing_profile(self) -> None:
        # Alice invites Bob to a DM.
        channel = self.make_request(
            "POST",
            "/_matrix/client/v3/createRoom",
            {
                "invite": [self.bob],
                "is_direct": True,
                "preset": "trusted_private_chat",
            },
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)
        room_id = channel.json_body["room_id"]

        # Bob accepts.
        self.helper.join(room_id, self.bob, tok=self.bob_tok)

        # Bob writes a normal message and a highlight message.
        self.helper.send_event(
            room_id,
            "m.room.message",
            {"body": "The pendulum draws the eye", "msgtype": "m.text"},
            tok=self.bob_tok,
        )
        self.helper.send_event(
            room_id,
            "m.room.message",
            {"body": "@alice", "msgtype": "m.text"},
            tok=self.bob_tok,
        )

        # Alice syncs. The room has a nonzero highlight count.
        channel = self.make_request(
            "GET",
            "/_matrix/client/v3/sync?timeout=100",
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)
        since = channel.json_body["next_batch"]
        counts = channel.json_body["rooms"]["join"][room_id]["unread_notifications"]
        self.assertEqual(counts, {"highlight_count": 1, "notification_count": 2})

        # Alice changes her displayname
        channel = self.make_request(
            "PUT",
            f"/_matrix/client/v3/profile/{self.alice}/displayname",
            {"displayname": "Alice [back next week]"},
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)

        # Alice syncs. The room's counts haven't changed.
        channel = self.make_request(
            "GET",
            f"/_matrix/client/v3/sync?since={since}&timeout=100",
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)
        counts2 = channel.json_body["rooms"]["join"][room_id]["unread_notifications"]
        self.assertEqual(counts2, {"highlight_count": 1, "notification_count": 2})

[t3chguy]

Arguably though it matches the spec:

https://spec.matrix.org/v1.9/client-server-api/#receiving-notifications

Servers MUST include the number of unread notifications in a client’s /sync stream, and MUST update it as it changes. Notifications are determined by the push rules which apply to an event.

When the user updates their read receipt (either by using the API or by sending an event), notifications prior to and including that event MUST be marked as read.

Key word being MUST. So the spec says in broad strokes that changing your profile (sending an m.room.member event into the room) MUST mark things as read.

[clokep]

Note that Synapse does not normally mark things as read if you send an event, i.e. https://github.com/matrix-org/synapse/issues/14837#issuecomment-1401922555...which doesn't match the spec (and it is confusing if it resets it here!)

[DMRobertson]

Re #9716 (comment):

Reproducible with a synapse test case:

This doesn't seem to be the full story. Tracing it through, the second sync in that response will hit this else branch:

synapse/synapse/storage/databases/main/event_push_actions.py

Lines 541 to 550 in 9c02ef2

	else:
	# If the user has no receipts in the room, retrieve the stream ordering for
	# the latest membership event from this user in this room (which we assume is
	# a join).
	event_id = self.db_pool.simple_select_one_onecol_txn(
	txn=txn,
	table="local_current_membership",
	keyvalues={"room_id": room_id, "user_id": user_id},
	retcol="event_id",
	)

and since the latest membership is now the displayname, we end up zeroing out the counts.

If you change the test to write an unthreaded read receipt prior to the highlighted message (hence not hitting this block), the test passes.

class TODOPickANameForTestCase(unittest.HomeserverTestCase):
    servlets = [
        admin.register_servlets_for_client_rest_resource,
        login.register_servlets,
        profile.register_servlets,
        receipts.register_servlets,
        room.register_servlets,
        sync.register_servlets,
    ]

    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
        self.alice = self.register_user("alice", "pass")
        self.alice_tok = self.login("alice", "pass")
        self.bob = self.register_user("bob", "pass")
        self.bob_tok = self.login("bob", "pass")

    def test_highlight_count_after_changing_profile_with_existing_receipt(self) -> None:
        """Attempt to reproduce https://github.com/matrix-org/synapse/issues/9716"""
        # Alice invites Bob to a DM.
        channel = self.make_request(
            "POST",
            "/_matrix/client/v3/createRoom",
            {
                "invite": [self.bob],
                "is_direct": True,
                "preset": "trusted_private_chat",
            },
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)
        room_id = channel.json_body["room_id"]

        # Bob accepts.
        self.helper.join(room_id, self.bob, tok=self.bob_tok)

        # Bob writes a message.
        bob_message_1 = self.helper.send_event(
            room_id,
            "m.room.message",
            {"body": "The pendulum draws the eye", "msgtype": "m.text"},
            tok=self.bob_tok,
        )["event_id"]

        # Alice sets a read receipt. (We do this because
        # - people notice the bug way after the room's creation, at which point you'll
        #   have read a previous message
        # - if we don't, we'll hit the else branch of _get_unread_counts_by_receipt_txn
        #   which will select Alice's latest membership as the point of reference
        channel = self.make_request(
            "POST",
            f"/_matrix/client/v3/rooms/{room_id}/receipt/m.read/{bob_message_1}",
            {},
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)

        # Bob pings Alice.
        self.helper.send_event(
            room_id,
            "m.room.message",
            {"body": "@alice", "msgtype": "m.text"},
            tok=self.bob_tok,
        )

        # Alice syncs. Bob's ping has caused a highlight.
        channel = self.make_request(
            "GET",
            "/_matrix/client/v3/sync?timeout=100",
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)
        since = channel.json_body["next_batch"]
        counts = channel.json_body["rooms"]["join"][room_id]["unread_notifications"]
        self.assertEqual(counts, {"highlight_count": 1, "notification_count": 1})

        # Alice changes her displayname
        new_displayname = "Alice [back next week]"
        channel = self.make_request(
            "PUT",
            f"/_matrix/client/v3/profile/{self.alice}/displayname",
            {"displayname": new_displayname},
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)

        # Alice syncs.
        channel = self.make_request(
            "GET",
            f"/_matrix/client/v3/sync?since={since}&timeout=100",
            access_token=self.alice_tok,
        )
        self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)

        # Alice sees her new membership
        ev = channel.json_body["rooms"]["join"][room_id]["timeline"]["events"][0]
        self.assertEqual(ev["sender"], self.alice, ev)
        self.assertEqual(ev["content"]["displayname"], new_displayname, ev)

        # The room's counts haven't changed.
        counts2 = channel.json_body["rooms"]["join"][room_id]["unread_notifications"]
        self.assertEqual(counts2, {"highlight_count": 1, "notification_count": 1})

[wrjlewis]

It's not completely clear what is actually happening behind the scenes here.
It looks like Synapse is behaving as expected in the unit tests.
Element Web counts notifications in some instances themselves, so worth double-checking what is happening there.
But not entirely certain on any presumptions yet, we should take some time to test this further.