Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle changes to required state config in sliding sync #17785

Merged
merged 15 commits into from
Oct 14, 2024

Conversation

erikjohnston
Copy link
Member

Fixes #17698

This handles required_state changes by checking if new state has been added to the config, and if so fetching and returning that from the current state.

This also takes care to ensure that given a state entry S that is added, removed and then re-added that we do not send S down a second time if there have been no changes to S in the current state. This is fine for Rust SDK (as it just remembers all state), but we might decide not to do this behaviour in the MSC. If we decide to always send down S then its easy enough to rip out all the code.

@erikjohnston erikjohnston marked this pull request as ready for review October 3, 2024 16:19
@erikjohnston erikjohnston requested a review from a team as a code owner October 3, 2024 16:19
synapse/handlers/sliding_sync/__init__.py Show resolved Hide resolved
Comment on lines +389 to +390
for event_type, state_key in db_to_json(row[1]):
state.setdefault(event_type, set()).add(state_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, whenever we db_to_json(...), we should scrutinize the output. We could add some asserts (or ignore invalid data with logging) to narrow the type to what we expect which gives us better types and we could've caught this earlier.

isinstance(event_type, str)
isinstance(state_key, str)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we could. Though we don't really do it elsewhere and they're covered by tests (and the data types are enforced by the DB).

Copy link
Contributor

@MadLittleMods MadLittleMods Oct 10, 2024

Choose a reason for hiding this comment

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

Wasn't this a mistake before that wasn't caught by either?

We previously assumed state_keys was a list but was just a single value and would have clobbered any required_state map with multiple state_keys

synapse/types/state.py Show resolved Hide resolved
synapse/handlers/sliding_sync/__init__.py Outdated Show resolved Hide resolved
tests/handlers/test_sliding_sync.py Outdated Show resolved Hide resolved
tests/handlers/test_sliding_sync.py Outdated Show resolved Hide resolved
synapse/handlers/sliding_sync/__init__.py Outdated Show resolved Hide resolved
tests/handlers/test_sliding_sync.py Outdated Show resolved Hide resolved
tests/handlers/test_sliding_sync.py Outdated Show resolved Hide resolved
MadLittleMods and others added 10 commits October 9, 2024 16:34
… required state (#17805)

Parameterize and add more tests for tracking changes to required state
(`_required_state_changes(...)`)

These are direct modifications to
#17785 and should be merged
into that PR.
@erikjohnston erikjohnston merged commit d025b5a into develop Oct 14, 2024
39 checks passed
@erikjohnston erikjohnston deleted the erikj/ss_required_state branch October 14, 2024 12:31
@ara4n ara4n changed the title Correctly changes to required state config in sliding sync Correctly handle changes to required state config in sliding sync Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sliding sync: handle room subscriptions changing required_state.
2 participants