Add experimental support for MSC4308: Thread Subscriptions extension to Sliding Sync when MSC4306 and MSC4186 are enabled.#18695
Conversation
7634587 to
ba41526
Compare
MadLittleMods
left a comment
There was a problem hiding this comment.
(I have looked things over but haven't given everything a proper review-level scrutiny)
ba41526 to
6165651
Compare
Works around #18712
6165651 to
f4cd180
Compare
|
previously-reviewed draft was very different, so putting back on the main queue |
| When creating a new sequence for a new stream, it will be necessary to advance it | ||
| so that position 1 is consumed. | ||
| DO NOT USE `START WITH 2` FOR THIS PURPOSE: | ||
| see https://github.com/element-hq/synapse/issues/18712 | ||
| Instead, use `SELECT nextval('sequence_name');` immediately after the | ||
| `CREATE SEQUENCE` statement. |
There was a problem hiding this comment.
Should we close #18712 once this PR is merged?
Unless we plan to fix it in some other fashion down the line.
There was a problem hiding this comment.
Hrmrmrmr. It feels like a design footgun that we could ideally resolve rather than having a hidden comment somewhere about what to do. But not sure if that's worth keeping an issue in the infinite backlog.
There was a problem hiding this comment.
IMO there's not too much benefit gained from properly fixing it vs. other things we could work on. So in a purely triage sense, I'd be in favour of closing the issue and leaving the comment as a helpful touch stone (one that may prompt folks to fix it in future if they really wanted to).
There was a problem hiding this comment.
Somewhat agreed, but the comment is not necessarily in a place you will notice (I'm not sure where you could do that, actually). The risk is that next time we come to adding a new one, we lose another couple of hours trying to track this issue down. But mrm I suppose this is probably the best we can do atm
| if limit <= 0: | ||
| raise SynapseError( | ||
| HTTPStatus.BAD_REQUEST, | ||
| "limit must be greater than 0", | ||
| errcode=Codes.INVALID_PARAM, | ||
| ) |
There was a problem hiding this comment.
negative=False on parse_integer will already do this for us.
There was a problem hiding this comment.
negative still allows 0 :(
There was a problem hiding this comment.
Ah, that's annoying.
Could you add a quick comment noting that this conditional is needed because of that?
…ions.py Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
| return HTTPStatus.OK, {} | ||
|
|
||
|
|
||
| class ThreadSubscriptionsPaginationRestServlet(RestServlet): |
There was a problem hiding this comment.
What do you think about adding rate-limiting to this handler?
There was a problem hiding this comment.
hmm... do we rate-limit any read operations already?
Just looking at https://spec.matrix.org/v1.15/client-server-api/#get_matrixclientv3roomsroomidmessages and sync and I'm not seeing them be rate limited.
Not that it's a terrible idea, but not sure how to balance with consistency (and the fact you could rate limit general requests with a load balancer e.g. nginx for instance)
There was a problem hiding this comment.
I mostly asked the question to check that it had been considered (which we tend to forget to do when adding new endpoints).
I'll accept the argument that the endpoint can be rate-limited by IP or access token in the load balancer for now, as I agree that we don't need anything more granular. We also have a MAX_LIMIT value defined, which prevents any one request from drawing too many resources.
|
(I assume Complement CI will fail until #18819 is merged?) |
# Synapse 1.139.0 (2025-09-30) ## Features - Add experimental support for [MSC4308: Thread Subscriptions extension to Sliding Sync](matrix-org/matrix-spec-proposals#4308) when [MSC4306: Thread Subscriptions](matrix-org/matrix-spec-proposals#4306) and [MSC4186: Simplified Sliding Sync](matrix-org/matrix-spec-proposals#4186) are enabled. ([\#18695](element-hq/synapse#18695)) - Update push rules for experimental [MSC4306: Thread Subscriptions](matrix-org/matrix-spec-proposals#4306) to follow a newer draft. ([\#18846](element-hq/synapse#18846)) - Add `get_media_upload_limits_for_user` and `on_media_upload_limit_exceeded` module API callbacks to the media repository. ([\#18848](element-hq/synapse#18848)) - Support [MSC4169](matrix-org/matrix-spec-proposals#4169) for backwards-compatible redaction sending using the `/send` endpoint. Contributed by @SpiritCroc @ Beeper. ([\#18898](element-hq/synapse#18898)) - Add an in-memory cache to `_get_e2e_cross_signing_signatures_for_devices` to reduce DB load. ([\#18899](element-hq/synapse#18899)) - Update [MSC4190](matrix-org/matrix-spec-proposals#4190) support to return correct errors and allow appservices to reset cross-signing keys without user-interactive authentication. Contributed by @tulir @ Beeper. ([\#18946](element-hq/synapse#18946)) ## Deprecations and Removals - Remove obsolete and experimental `/sync/e2ee` endpoint. ([\#18583](element-hq/synapse#18583)) # Synapse 1.138.0 (2025-09-09) ## Features - Support for the stable endpoint and scopes of [MSC3861](matrix-org/matrix-spec-proposals#3861) & co. ([\#18549](element-hq/synapse#18549))
# Synapse 1.139.0 (2025-09-30) ## Features - Add experimental support for [MSC4308: Thread Subscriptions extension to Sliding Sync](matrix-org/matrix-spec-proposals#4308) when [MSC4306: Thread Subscriptions](matrix-org/matrix-spec-proposals#4306) and [MSC4186: Simplified Sliding Sync](matrix-org/matrix-spec-proposals#4186) are enabled. ([\#18695](element-hq/synapse#18695)) - Update push rules for experimental [MSC4306: Thread Subscriptions](matrix-org/matrix-spec-proposals#4306) to follow a newer draft. ([\#18846](element-hq/synapse#18846)) - Add `get_media_upload_limits_for_user` and `on_media_upload_limit_exceeded` module API callbacks to the media repository. ([\#18848](element-hq/synapse#18848)) - Support [MSC4169](matrix-org/matrix-spec-proposals#4169) for backwards-compatible redaction sending using the `/send` endpoint. Contributed by @SpiritCroc @ Beeper. ([\#18898](element-hq/synapse#18898)) - Add an in-memory cache to `_get_e2e_cross_signing_signatures_for_devices` to reduce DB load. ([\#18899](element-hq/synapse#18899)) - Update [MSC4190](matrix-org/matrix-spec-proposals#4190) support to return correct errors and allow appservices to reset cross-signing keys without user-interactive authentication. Contributed by @tulir @ Beeper. ([\#18946](element-hq/synapse#18946)) ## Deprecations and Removals - Remove obsolete and experimental `/sync/e2ee` endpoint. ([\#18583](element-hq/synapse#18583)) # Synapse 1.138.0 (2025-09-09) ## Features - Support for the stable endpoint and scopes of [MSC3861](matrix-org/matrix-spec-proposals#3861) & co. ([\#18549](element-hq/synapse#18549))
| un_partial_stated_rooms_key = self.store.get_un_partial_stated_rooms_token( | ||
| self._instance_name | ||
| ) | ||
| thread_subscriptions_key = self.store.get_max_thread_subscriptions_stream_id() |
There was a problem hiding this comment.
See #19365 (comment) for a discussion on whether we should be using MultiWriterStreamToken instead of a single stream_id int.
Closes: #18436
Implements: matrix-org/matrix-spec-proposals#4308
Follows: #18674
This pull request is intended for commit-by-commit review.
This pull request adds an extension to Sliding Sync and a companion endpoint needed for backpaginating missed thread subscription changes,
as described in MSC4308