Repository navigation
MSC4140: drop delayed state interference - #19495
Conversation
No longer cancel a user's delayed state events by a more recent state event sent from a different user. This behaviour has been delegated to an "alternative" in the delayed event MSC for some time now, and is not known to be used by any module other than experimental iterations of MatrixRTC.
reivilibre
left a comment
reivilibre
left a comment
There was a problem hiding this comment.
This makes sense, just need to go through the slightly tedious motions for the schema version, I think
| CREATE INDEX delayed_events_is_processed ON delayed_events (is_processed); | ||
| CREATE INDEX delayed_events_room_state_event_idx ON delayed_events (room_id, event_type, state_key) WHERE state_key IS NOT NULL; | ||
|
|
||
| -- TODO: delete this table in the next Synapse release that can handle a breaking DB change |
There was a problem hiding this comment.
This is possibly better tracked as a GitHub issue instead of a comment here.
More concretely, we need to study https://element-hq.github.io/synapse/latest/development/database_schema.html#synapse-schema-versions
There's a worked example with a table removal. Since I think it's fine to have degraded rollback support for this experimental feature (as in, I think it's probably OK to stop reading and writing to this table at the same time), I think for us this is something along the lines of:
-
SCHEMA_VERSIONneeds to be increased now, with a comment on the version saying that in the newSCHEMA_VERSIONwe no longer use this table. -
In a later version, we can make
SCHEMA_COMPAT_VERSIONmatch that as we drop the table, preventing rollback to the olderSCHEMA_VERSION.
There was a problem hiding this comment.
SCHEMA_VERSIONneeds to be increased now, with a comment on the version saying that in the newSCHEMA_VERSIONwe no longer use this table.
Since the schema isn't actually changed by this PR, would it be acceptable to put that comment on the latest SCHEMA_VERSION (which is a newer version than when the table was created) instead of creating an "empty" bump?
There was a problem hiding this comment.
This question still stands.
There was a problem hiding this comment.
Sorry I didn't see this earlier. Please do make use of the re-request button :-)
There was a problem hiding this comment.
Anything to learn here and better document in docs/development/database_schema.md ? (a new worked example?)
There was a problem hiding this comment.
Maybe what's worth mentioning is that the DB schema version isn't the version of the physical DB schema itself, but a version of the intended usage / guarantees of the schema. Realizing that is what made the docs "click" for me.
Co-authored-by: Olivier 'reivilibre' <olivier@librepush.net>
Don't test that a user's delayed state events are cancelled by a more recent state event from a different user. That behaviour [has been delegated to an "alternative"] (matrix-org/matrix-spec-proposals@3efecea) in the delayed event MSC4140 for some time now, and may be removed from Synapse ([Synapse PR](element-hq/synapse#19495))
Resolve the conflict in the delayed events handler: - Keep the removal of `notify_new_event`, `_unsafe_process_new_event` and `_handle_state_deltas`. Develop only touched them to swap `Sentinel.UNSET_SENTINEL` for `Absent`. - Imports: take develop's `Codes` and `SynapseError`, which `add` now needs for its 403 `M_FORBIDDEN` and `DELAY_TOO_LARGE` errors, and drop `Measure` and `Absent`, which only the removed code used.
Include new methods & tests from element-hq#20247
Use a DB schema version update to document the change in behaviour, rather than a TODO comment on the table itself.
The skips added by that PR are no longer needed
Remove test that was modified by element-hq#19354 but had been removed by this PR
When a batch of timed out delayed events includes state events, simply send those state events in order with all other events to be sent, rather than sending only the first state event per matching state key & doing so after sending message events. This is safe now that delayed state event intererence has been dropped (see element-hq#19495).
4 participants
No longer cancel a user's delayed state events by a more recent state event sent from a different user. This behaviour has been delegated to an "alternative" in the delayed event MSC for some time now, and is not known to be used by any module other than experimental iterations of MatrixRTC.
See also: matrix-org/complement#845
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.