Marker events as state - MSC2716 #371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MadLittleMods
commented
May 12, 2022
| // Paginate the /messages endpoint until we find all of the expectedEventIds | ||
| // (order does not matter). If any event in denyListEventIDs is found, an error | ||
| // will be thrown. | ||
| func paginateUntilMessageCheckOff(t *testing.T, c *client.CSAPI, roomID string, fromPaginationToken string, expectedEventIDs []string, denyListEventIDs []string) { |
There was a problem hiding this comment.
This function is originally from #214 but saw that we could use it here before that merges
This was referenced May 13, 2022
MadLittleMods
commented
May 19, 2022
… current state to process See matrix-org/synapse#12718 (comment)
MadLittleMods
commented
May 19, 2022
| // Marker events should have unique state_key so they all show up in the current state to process. | ||
| unique_state_key := getTxnID("marker_state_key") | ||
| // We can't use as.SendEventSynced(...) because application services can't use the /sync API. | ||
| markerSendRes := as.MustDoFunc(t, "PUT", []string{"_matrix", "client", "r0", "rooms", roomID, "state", markerEvent.Type, unique_state_key}, client.WithJSONBody(t, markerEvent.Content)) |
There was a problem hiding this comment.
Sending marker events as state now so they are always able to be seen by homeservers (not lost in some timeline gap).
Marker events should be sent with a unique state_key so that they can all resolve in the current state to easily be discovered.
- If we re-use the same
state_key(like""), then we would have to fetch previous snapshots of state up through time to find all of the marker events. This way we can avoid all of that. - Also avoids state resolution conflicts where only one of the marker events win
There was a problem hiding this comment.
From a design/protocol perspective, this feels sub-optimal. What you really wanted was something like MSC2836 which would allow you to walk back up marker events over federation without needing to put things into state, but oh well.
MadLittleMods
changed the title
kegsay
approved these changes
May 20, 2022
| // Marker events should have unique state_key so they all show up in the current state to process. | ||
| unique_state_key := getTxnID("marker_state_key") | ||
| // We can't use as.SendEventSynced(...) because application services can't use the /sync API. | ||
| markerSendRes := as.MustDoFunc(t, "PUT", []string{"_matrix", "client", "r0", "rooms", roomID, "state", markerEvent.Type, unique_state_key}, client.WithJSONBody(t, markerEvent.Content)) |
There was a problem hiding this comment.
From a design/protocol perspective, this feels sub-optimal. What you really wanted was something like MSC2836 which would allow you to walk back up marker events over federation without needing to put things into state, but oh well.
MadLittleMods
added a commit
to matrix-org/synapse
that referenced
this pull request
May 24, 2022
Sending marker events as state now so they are always able to be seen by homeservers (not lost in some timeline gap). Part of [MSC2716](matrix-org/matrix-spec-proposals#2716) Complement tests: matrix-org/complement#371 As initially discussed at matrix-org/matrix-spec-proposals#2716 (comment) and matrix-org/matrix-spec-proposals#2716 (comment) When someone joins a room, process all of the marker events we see in the current state. Marker events should be sent with a unique `state_key` so that they can all resolve in the current state to easily be discovered. Marker events as state - If we re-use the same `state_key` (like `""`), then we would have to fetch previous snapshots of state up through time to find all of the marker events. This way we can avoid all of that. This PR was originally doing this but then thought of the smarter way to tackle in an [out of band discussion with @erikjohnston](https://docs.google.com/document/d/1JJDuPfcPNX75fprdTWlxlaKjWOdbdJylbpZ03hzo638/edit#bookmark=id.sm92fqyq7vpp). - Also avoids state resolution conflicts where only one of the marker events win As a homeserver, when we see new marker state, we know there is new history imported somewhere back in time and should process it to fetch the insertion event where the historical messages are and set it as an insertion extremity. This way we know where to backfill more messages when someone asks for scrollback.
|
Thanks for the review @kegsay 🦢 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Synapse change: matrix-org/synapse#12718
Part of MSC2716
We're testing to make sure historical messages show up for a remote federated homeserver even when the homeserver is missing the part of the timeline where the marker events were sent and it paginates before they occured to see if the history is available. Making sure the homeserver processes all of the markers from the current state instead of just when it sees them in the timeline.
Sending marker events as state now so they are always able to be seen by homeservers (not lost in some timeline gap).
Marker events should be sent with a unique
state_keyso that they can all resolve in the current state to easily be discovered.state_key(like""), then we would have to fetch previous snapshots of state up through time to find all of the marker events. This way we can avoid all of that.As a homeserver, when we see new marker state, we know there is new history imported somewhere back in time and should process it to fetch the insertion event where the historical messages are and set it as an insertion extremity. This way we know where to backfill more messages when someone asks for scrollback.
Dev notes