Threaded receipt EDU shape down /sync makes it impossible to express certain valid receipts

[kegsay]

Threaded read receipts got added in MSC3771 - they look like this down /sync:

{
  "content": {
    "$thread_reply": {
      "m.read": {
        "@rikj:jki.re": {
          "ts": 1436451550453,
          "thread_id": "$thread_root" // or "main" or absent
        }
      }
    }
  },
  "room_id": "!jEsUZKDJdhlrceRyVU:example.org",
  "type": "m.receipt"
}

It is my understanding that receipts form a key-value store, where the key is a 3-uple of (user_id, room_id, thread_id), with the value of the read (event_id, ts). This means a user can have a N+1 receipts in a room, one for each thread ID and one unthreaded receipt (ignoring private receipts). However, the JSON shape restricts the ability to express the following data:

room | user | event | thread
-----------------------------
!foo | @bob | $abc  | NULL
!foo | @bob | $abc  | "some_id"

It restricts it because the JSON shape is keyed off the 2-uple (event_id, user_id) - the key is too broad. The net result is that in the sliding sync proxy and Synapse, these receipts are inexpressible and one will be dropped in a undefined manner.

It may be that it's silly for clients to send this particular set of receipts, but given how we want to work with thread-aware and non-thread-aware clients, it's not unrealistic that clients may decide to send 2 receipts for an event: one for "unthreaded" for the benefit of thread unaware clients and one for the thread_id for thread-aware clients. If they do this, things break.

Element-Web does exactly this:

• Click on the button : Mark room as read => unthreaded receipt
• When you open a room or a thread, interact with the composer etc => threaded receipt

and it's possible to send both for the same event ID:

the normal RR could be sent for the main/thread timeline, then you send an unthreaded to mark all timelines as read on the same event id if it is the latest in the room

[clokep]

It may be that it's silly for clients to send this particular set of receipts, but given how we want to work with thread-aware and non-thread-aware clients, it's not unrealistic that clients may decide to send 2 receipts for an event: one for main for the benefit of thread unaware clients and one for the thread_id for thread-aware clients. If they do this, things break.

This is invalid -- there can be two receipts, but they would be an unthreaded receipt and some_id. The event can't be on both the main thread and some_id thread.

---

The shape is lacking though and you're correct though, we were constrained by the current shape of receipts / EDUs down sync. (I thought this was called out in the MSC, but seems I never added this to the potential issues section.)

[richvdh]

I was just writing the same as clokep, but he's faster than me :)

This means a user can have a N+1 receipts in a room

N+2, I think? One for each thread, one for the main pseudo thread, and one unthreaded ?

[kegsay]

I checked the MSC and PR comments but failed to see anyone mention it, hence raising this issue.

Given main is approximately the same as unthreaded:

there can be two receipts, but they would be an unthreaded receipt and some_id. The event can't be on both the main thread and some_id thread.

then sure, s/main//g everywhere and it's the same problem? We cannot express:

room | user | event | thread
-----------------------------
!foo | @bob | $abc  | NULL
!foo | @bob | $abc  | "some_id"

[clokep]

Additionally, federation has a similar issue since they're mapped room -> type -> user. Synapse has some complicated code to ensure that receipts on different threads don't stomp each other. Instead of being able to send a single receipt EDU the best you can now do is N EDUs where N is the maximum threaded receipts in the same room by the same user (or something like that).

[kegsay]

So the combination of:

• Element-Web does send threaded/unthreaded receipts for potentially the same event,
• Synapse clobbering receipts when this happens (last write wins) - Add tests for MSC4102 complement#709

means that I think this is a valid concern, because receipt data is lost between users irreversibly. I don't know how the complex unread notification code will handle things if receipts are being dropped seemingly randomly.

[dbkr]

If I've understood this correctly, this is very much a theoretical problem since an unthreaded receipt is implicitly a receipt for all threads, so a threaded receipt for the same event is irrelevant. However, the spec doesn't make this clear and also doesn't specify that the HS should give the unthreaded receipt precedence, so we should make an MSC to fix that, which I believe is what Kegan is doing.

Any client that supports threaded receipts will have to support both anyway and so should already be doing the right thing.

[kegsay]

@dbkr's insight is the key to fixing this, because it means unthreaded (missing thread_id) receipts can safely replace the threaded version without losing data.

It looks like this problem was independently discovered by @andybalaam in #1684 and the solution proposed was to just not batch EDUs, which is simply unnecessary given with the correct replacement rules you can safely batch.

[richvdh]

I'm struggling to grasp at a glance what action this needs.

• It's labelled as clarification so it's just a matter of making the spec match reality? If so, what's the reality? I'm not convinced this is accurate.
• So do we need an MSC to actually change what the spec says here?