MSC4308: Thread Subscriptions extension to Sliding Sync

[reivilibre]

Depends-on: #4186, #4306

Rendered

[MadLittleMods]

Are these on purpose? (applies elsewhere)

Suggested change

	to be sent to the client. \
	to be sent to the client.

[reivilibre]

This is the most sane of the two ways of adding a line break in the more standard variants of Markdown.
(GFM is I think the only notable dialect that doesn't need this)

[MadLittleMods]

Based on prior art, feels like prev_batch/next_batch is the pattern to align with.

Ex.

• prev_batch from /sync responses are supposed to be used with /messages?from=xxx&to=xxx
• next_batch from /sync responses are supposed to be used with /sync?since=xxx and /messages?from=xxx&to=xxx
• pos in Sliding sync responses are supposed to be used with Sliding Sync /sync?since=xxx
• start/end from /messages responses are supposed to be used with /messages?from=xxx&to=xxx
• next_batch from /threads responses is supposed to be used with /threads?from=xxx
• next_token from /notifications responses is supposed to be used with /notifications?from=xxx
• next_batch from /hierarchy responses is supposed to be used with /hierarchy?from=xxx

Overall, I find our API surface a bit confusing on how we've named fields.

We probably do want to include pagination tokens for both sides of the response (prev/next) so clients can fill in the gap from both directions as desired.

[bnjbvr]

Out of curiosity, what's the use case for a next token? i.e., in which cases is one interested in getting a thread subscriptions and the ones after it? I feel that we're not going to need this in the short term, and, if we need it, we could also add it when it's actually justified by a use case. So, as a wish to keep the API as minimal as it can be, I'd be in favor of not adding support for next as well as a direction parameter, unless it's strongly motivated by a real-world use case.

[reivilibre]

I'd also rather not add a 'forward' direction. Maybe we can add the dir=b parameter now and require it to always point backwards.

I am struggling to line up the semantics I want here with the existing API patterns, which is namely tracking and filling a gap (i.e. there are bounds on both sides).

The clients should not really care about the direction of filling, because the time of a subscription is not really important, just the latest state of it.

[reivilibre]

I have changed the /thread_subscriptions API to use from=xxx&to=xxx&dir=b, but wondering how we should pass back the pair of tokens from Sliding Sync? I've added a new proposal for that but I'm not sure it's the most satisfying (though couldn't see endpoints that do this already for double-ended bounds)

[MadLittleMods]

I love the bulk endpoint 👍 but is there also a way to get thread_subscriptions for a single room?

I can definitely picture some UI which shows the subscribed threads in a room and it would suck to have to wait for the whole room list to paginate in before you could see the list for the single room you're staring at.

The dumb-simple solution is a filter parameter for this endpoint instead of a separate endpoint.

[reivilibre]

There isn't, but we expect clients to be needing to cache this information locally anyway, so it'd cover a very tiny surface (i.e. during the time we haven't managed to fully sync yet) on a UI I'm not actually sure anyone would implement (rather than 'list of my subscriptions in this room in time order of the subscription', wouldn't you instead be interested in 'list of most recent threads in this room, filtered by whether I'm subscribed').

So I kinda felt it was not worth adding extra API surface here given I don't have a use case in mind for it. If someone disagrees this could easily be rectified in the future.

[reivilibre]

Thinking on this, this also means servers need to add indices for this and optimise a bit for it. It's not really for free. And if we don't think of a valid use for it, I guess that's actually harmful

[MadLittleMods]

If I come back to my desktop after a week of working on my phone on a holiday, my desktop client is going to need to catch up on the threads I'm subscribed to. If I click into a room and open a threads panel, I will be waiting until my clients catches up on everything whereas it could use the dedicated room endpoint to get everything directly ahead of the entire list on my account.

"filtered by whether I'm subscribed" is the key here. I need to know whether some threads are subscribed.

[reivilibre]

Hmm, I see what you'd mean, I'd kinda like a client dev who is implementing this to provide some kind of opinion though on what would work for them.

An ugly but maybe-valid workaround in that case could be to backpaginate the threads in general and check the subscriptions one-by-one manually. Given HTTP pipelining and multiplexing, there are probably worse crimes.
I think more effective would be to inline the subscription directly in the APIs where that makes sense. e.g. filtering the threads to only subscribed ones, directly in /threads, or at least returning the state there.

[bnjbvr]

If I come back to my desktop after a week of working on my phone on a holiday, my desktop client is going to need to catch up on the threads I'm subscribed to. If I click into a room and open a threads panel, I will be waiting until my clients catches up on everything whereas it could use the dedicated room endpoint to get everything directly ahead of the entire list on my account.

Remember how thread subscriptions are entirely controlled by clients, and never created by the server? If you came back after a holiday and didn't open any client for a week, then you won't have any new subscriptions, by definition. Then, your client would catch up on the threads (not the thread subscriptions), using either automatic back-pagination for all the threads (bleh) or a new threads catchup mechanism (yay), and then only would it recompute new subscriptions for the threads that have been active during your holiday.

If other devices have sync'd and did subscribe you to threads: you'd receive the updates from the sliding sync extension, and then if there were too many of them, you'd use the global endpoints.

Having a room-specific endpoint might likely make the second case above slightly faster (if the other device subscribed to many other threads on many other rooms). But if this endpoint's processing speed is Good Enough™ (viz. not blocked on network/federation), I imagine that it would be sufficient to have only the global feed, for the time being. Does it make sense?

An ugly but maybe-valid workaround in that case could be to backpaginate the threads in general and check the subscriptions one-by-one manually. Given HTTP pipelining and multiplexing, there are probably worse crimes.

For what it's worth, I did experiment with backpaginating all of a room's threads, when implementing a dummy activity center in the SDK, and this was way too slow for a room like MatrixHQ (of course this is the extreme edge case).

I think more effective would be to inline the subscription directly in the APIs where that makes sense. e.g. filtering the threads to only subscribed ones, directly in /threads, or at least returning the state there.

Maybe this could be part of the bundled thread summary, for thread roots, next to the current_user_participated which really thread subscriptions are a generalization of?

Also: note that filtering to only subscribed ones wouldn't give you the information that you may have unsubscribed from another device in the meanwhile.

[MadLittleMods]

Perhaps the bulk endpoint should work like this:

We could implement a bulk /messages endpoint, where the client would specify multiple rooms and prev_batch tokens.

-- MSC4186

And this also gets around the problem where you can't get to the thread subscriptions that you care about without paginating through the bulk. This way, you can specify the rooms you want to get more thread subscriptions in.

Not sure how fetching thread subscriptions across all rooms would look like yet (many options).

Also being explored in element-hq/synapse#19005 (comment)

[MadLittleMods]

Per my other comment,

We probably do want to include pagination tokens for both sides of the response (prev/next) so clients can fill in the gap from both directions as desired.

This would also require a dir direction parameter. I'm not sure it's necessary to refactor to a from/to pattern like /messages but maybe more obvious if they behave the same.

We should also clarify the token bounds. Synapse reference for other places:

• https://github.com/element-hq/synapse/blob/d27438bc253b772cadaa7cbd914bbd5631ee3b05/synapse/storage/databases/main/stream.py#L690-L691
• https://github.com/element-hq/synapse/blob/d27438bc253b772cadaa7cbd914bbd5631ee3b05/synapse/handlers/sliding_sync/__init__.py#L202

[bnjbvr]

Thanks, I like this updated version a lot!

[bnjbvr]

Out of curiosity, do you have ideas for other sibling fields at this level? Or is this for symmetry with other response subparts?

(The one idea that comes to mind is that the request/response parts could be a subset of a new threads field, something like:

 "threads": {
    "subscriptions": {
      "!roomId1:example.org": { ... }
    }
  }

)

[reivilibre]

Other than pos which now exists (although might be renamed, c.f. the other thread), the idea was to not block off extensibility; i.e. if we just flattened this struct then we would have no avenue to extend this Sliding Sync extension.

threads -> subscriptions might work, but by the same token, the extension is called threads_subscriptions, so it is weird for the answer to not match the extension name (and I think the Sliding Sync MSC actually says they should match). It would still have the same extensibility problem :/

[bnjbvr]

Hmm, the wording is confusing here as it's talking about the sliding sync response. We might want to precise here again that it's the pos value from the latest request?

[reivilibre]

Tried (and maybe failed) to improve with 4d88858

[bnjbvr]

Should we add a client behavior section defining that ideally the thread subscriptions would all be persisted on the client's side, to avoid having to call into the GET endpoint for individual thread subscriptions when computing notifications locally (notably for e2ee rooms)? The "ideally" in my sentence suggests this could be worded as a "MAY".

Then I guess, to implement this optimization correctly, a client would have to remember if it's synchronized all threads subscriptions yet, with the following assumptions:

• when a SSS connection has been reset (via a new pos), a client should assume it doesn't know about all the thread subscriptions yet (set an hypothetical boolean known_thread_subscriptions to false)
• after receiving a response and paginating all the previous unknown thread subscriptions, the client should assume it does know with certainty about all the thread subscriptions (set known_thread_subscriptions to true)
• when getting a thread subscription: if !known_thread_subscriptions, a GET request should be sent to figure whether a thread has a subscription or not; otherwise, the state synchronized locally can be used.

[bnjbvr]

For what it's worth, my implementation will likely now store all the bumpstamps in the database (so as to be able to order the latest sub vs unsub, notably). This creates a small discrepancy, when using the endpoints introduced from MSC4306:

• when we use the PUT or DELETE endpoints, we don't receive a bump_stamp. In this case, it's expected that the bumpstamp will come with the remote echo from the next sync. If we want to implement "local echo" behvaior when using the PUT or DELETE endpoints, then we'd need to store the latest status (subscribed/unsubscribed) in the database, and we wouldn't have a new bumpstamp. I think that in this case, it might be sufficient to not update the bumpstamp.
• When using the GET endpoint, we don't receive a bumpstamp, but that's fine: it's supposed to be the latest version anyways. Ideally, we'd cache the result in database too, here. And maybe we can perform the same hack as indicated above (keep the previous bumpstamp stored in DB, and only update the subscription status, if it's changed).

TL;DR: adding bump_stamp to the MSC4306 endpoints might be better for API consistency, but not sure it's achieving anything useful, considering the hack described above.

[bnjbvr]

• it'd be nice to add the enabled: boolean here too, for consistency with other extensions (and because the synapse impl requires it, and I was confused as to why my request wasn't taken into account xD)
• also i see in the impl that limit is required, but I wonder if it should? I didn't put it first, assuming the server would give me a nice default value.

[reivilibre]

Added the enabled flag, sorry for forgetting it!

The limit is now opened as a bug in Synapse.