Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow non-member state sent in room batch to resolve for historic events (MSC2716) #12329

Merged
merged 3 commits into from
Apr 4, 2022
Merged

Allow non-member state sent in room batch to resolve for historic events (MSC2716) #12329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d770c5
Allow non-member state sent in room batch to resolve for historic events
MadLittleMods Mar 30, 2022
ea0c99d
Add changelog
MadLittleMods Mar 30, 2022
5151de0
Remove mentions of outlier having a role with the historical state chain
MadLittleMods Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12329.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix non-member state events not resolving for historical events when used in [MSC2716](https://github.com/matrix-org/matrix-spec-proposals/pull/2716) `/batch_send` `state_events_at_start`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally not setting these things as outliers seem sensible, but I'm not quite following the justification.

Shout with any more clarification questions 👍

38 changes: 12 additions & 26 deletions synapse/handlers/room_batch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@ async def persist_state_events_at_start(
) -> List[str]:
"""Takes all `state_events_at_start` event dictionaries and creates/persists
them in a floating state event chain which don't resolve into the current room
state. They are floating because they reference no prev_events and are marked
as outliers which disconnects them from the normal DAG.
state. They are floating because they reference no prev_events which disconnects
them from the normal DAG.

Args:
state_events_at_start:
Expand Down Expand Up @@ -213,31 +213,23 @@ async def persist_state_events_at_start(
room_id=room_id,
action=membership,
content=event_dict["content"],
# Mark as an outlier to disconnect it from the normal DAG
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
# and not show up between batches of history.
outlier=True,
Copy link
Contributor Author

@MadLittleMods MadLittleMods Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these as outliers because filter_events_for_client filters out all outlier state except for out-of-band membership. And we want all of the state_events_at_start to resolve when a historical event is pulled.

This state chain is still disconnected from the normal DAG because the first event has empty prev_events so they state just floats in a chain of its own.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to understand how the diagram relates to the change here. Which of the events on the diagram correspond to this code path?

Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just giving it as a reference. It shows that the state chain floats by no prev_events and that the state chain is disconnected from the historical message chain (no prev_event edges connect them).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so this code is handling the events in the yellow boxes labelled batch0, batch1, batch2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

persist_state_events_at_start is handling the state events above the yellow boxes (m.room.member events in the diagram). The historical state chain.

The yellow boxes are the historical messages chains

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

persist_state_events_at_start is handling the state events above the yellow boxes (m.room.member events in the diagram). The historical state chain.

well in that case, this seems wrong. those events seem to be random state events where we haven't wired them up to the event dag? why would we have the state at those events?

Either I'm severely understanding your diagram, or something is odd here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well in that case, this seems wrong. those events seem to be random state events where we haven't wired them up to the event dag?

Correct, they float in their own chain.

why would we have the state at those events?

But we technically do have the state for them because we're manually providing it via state_event_ids

historical=True,
# Only the first event in the state chain should be floating.
# The rest should hang off each other in a chain.
allow_no_prev_events=index == 0,
prev_event_ids=prev_event_ids_for_state_chain,
# Since each state event is marked as an outlier, the
# `EventContext.for_outlier()` won't have any `state_ids`
# set and therefore can't derive any state even though the
# prev_events are set. Also since the first event in the
# state chain is floating with no `prev_events`, it can't
# derive state from anywhere automatically. So we need to
# set some state explicitly.
# The first event in the state chain is floating with no
# `prev_events` which means it can't derive state from
# anywhere automatically. So we need to set some state
# explicitly.
#
# Make sure to use a copy of this list because we modify it
# later in the loop here. Otherwise it will be the same
# reference and also update in the event when we append later.
# reference and also update in the event when we append
# later.
state_event_ids=state_event_ids.copy(),
)
else:
# TODO: Add some complement tests that adds state that is not member joins
# and will use this code path. Maybe we only want to support join state events
# and can get rid of this `else`?
(
event,
_,
Expand All @@ -246,21 +238,15 @@ async def persist_state_events_at_start(
state_event["sender"], app_service_requester.app_service
),
event_dict,
# Mark as an outlier to disconnect it from the normal DAG
# and not show up between batches of history.
outlier=True,
historical=True,
# Only the first event in the state chain should be floating.
# The rest should hang off each other in a chain.
allow_no_prev_events=index == 0,
prev_event_ids=prev_event_ids_for_state_chain,
# Since each state event is marked as an outlier, the
# `EventContext.for_outlier()` won't have any `state_ids`
# set and therefore can't derive any state even though the
# prev_events are set. Also since the first event in the
# state chain is floating with no `prev_events`, it can't
# derive state from anywhere automatically. So we need to
# set some state explicitly.
# The first event in the state chain is floating with no
# `prev_events` which means it can't derive state from
# anywhere automatically. So we need to set some state
# explicitly.
#
# Make sure to use a copy of this list because we modify it
# later in the loop here. Otherwise it will be the same
Expand Down