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

Fix bug which caused rejected events to be stored with the wrong room state #6320

Merged
merged 4 commits into from Nov 6, 2019

Conversation

@richvdh
Copy link
Member

richvdh commented Nov 4, 2019

The problem here was the code in StateStore which assumed that prev_group gave the state before the event being persisted. Whilst this may (sometimes) have been true for state events, it was never true for message events.

I'm not entirely sure if this is the best way to fix it, but the general idea is to rewrite compute_event_context to ensure that we always have a state group for the state before the event being persisted (and to store it in a new field in the EventContext). That also allows us to rewrite it to reduce a bunch of duplication between the code paths.

Fixes #6289.

This PR builds on #6319.

@richvdh richvdh changed the title ### Pull Request Checklist Fix bug which caused rejected events to be stored with the wrong room state Nov 4, 2019
@richvdh richvdh force-pushed the rav/event_context/2 branch from 73cd3c2 to a80143f Nov 4, 2019
@richvdh richvdh requested a review from matrix-org/synapse-core Nov 4, 2019
@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Nov 5, 2019

Do we have any unit tests for compute_event_context?

@richvdh

This comment has been minimized.

Copy link
Member Author

richvdh commented Nov 5, 2019

Do we have any unit tests for compute_event_context?

there are a bunch in https://github.com/matrix-org/synapse/blob/develop/tests/test_state.py.

I guess I know where that question is leading...

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Nov 5, 2019

Do we have any unit tests for compute_event_context?

there are a bunch in https://github.com/matrix-org/synapse/blob/develop/tests/test_state.py.

I guess I know where that question is leading...

Just adding some checks for the new fields in those tests would be good 😇

richvdh added 2 commits Nov 4, 2019
Fixes a bug where rejected events were persisted with the wrong state group.

Also fixes an occasional internal-server-error when receiving events over
federation which are rejected and (possibly because they are
backwards-extremities) have no prev_group.

Fixes #6289.
@richvdh richvdh force-pushed the rav/event_context/2 branch from 37ec6ee to ba3a5e8 Nov 5, 2019
@richvdh

This comment has been minimized.

Copy link
Member Author

richvdh commented Nov 5, 2019

tests (and a lying docstring) updated in ba3a5e8

@richvdh richvdh merged commit 807ec3b into develop Nov 6, 2019
20 checks passed
20 checks passed
buildkite/synapse Build #5425 passed (24 minutes, 37 seconds)
Details
buildkite/synapse/check-sample-config Passed (2 minutes)
Details
buildkite/synapse/check-style Passed (1 minute, 45 seconds)
Details
buildkite/synapse/isort Passed (15 seconds)
Details
buildkite/synapse/mypy Passed (22 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (43 seconds)
Details
buildkite/synapse/packaging Passed (25 seconds)
Details
buildkite/synapse/pipeline Passed (3 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (18 minutes, 26 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (6 minutes, 54 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (22 minutes, 17 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (6 minutes, 57 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (18 minutes, 44 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 11 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 19 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 42 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (1 minute, 44 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (14 minutes, 58 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (12 minutes, 37 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (13 minutes, 50 seconds)
Details
@richvdh richvdh deleted the rav/event_context/2 branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.