Skip to content
This repository was 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
MadLittleMods merged 3 commits into from
Apr 4, 2022
Merged

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

Changes from 1 commit
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
6 changes: 0 additions & 6 deletions synapse/handlers/room_batch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,6 @@ 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
Comment thread
MadLittleMods marked this conversation as resolved.
# and not show up between batches of history.
outlier=True,
Copy link
Copy Markdown
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
Copy Markdown
Member

@richvdh richvdh Apr 1, 2022

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
Copy Markdown
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
Copy Markdown
Member

@richvdh richvdh Apr 4, 2022

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
Copy Markdown
Contributor Author

@MadLittleMods MadLittleMods Apr 4, 2022

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
Copy Markdown
Member

@richvdh richvdh Apr 5, 2022

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
Copy Markdown
Contributor Author

@MadLittleMods MadLittleMods Apr 5, 2022

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.
Expand Down Expand Up @@ -246,9 +243,6 @@ 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.
Expand Down