From f98c6910118d1140e4fb325ee11a411fae910cee Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 14 Jul 2022 17:22:17 +0100 Subject: [PATCH 01/11] Fix bug where the state deltas were incorrect --- synapse/state/__init__.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index 9f0a36652c25..91c41d8ec4b9 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -769,6 +769,11 @@ def _make_state_cache_entry( delta_ids: Optional[StateMap[str]] = None for old_group, old_state in state_groups_ids.items(): + if old_state.keys() - new_state.keys(): + # Currently we don't support deltas that remove keys from the state + # map. + continue + n_delta_ids = {k: v for k, v in new_state.items() if old_state.get(k) != v} if not delta_ids or len(n_delta_ids) < len(delta_ids): prev_group = old_group From efd5c3213af04a3811feb68ed84dc59f7365ff16 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 14 Jul 2022 17:27:20 +0100 Subject: [PATCH 02/11] Fix bug where we didn't calculate the state correctly for new events This was introduced in #13267 --- synapse/storage/controllers/persist_events.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/storage/controllers/persist_events.py b/synapse/storage/controllers/persist_events.py index af65e5913b4b..07c75e86868f 100644 --- a/synapse/storage/controllers/persist_events.py +++ b/synapse/storage/controllers/persist_events.py @@ -934,6 +934,8 @@ async def _get_new_state_after_events( state_res_store=StateResolutionStore(self.main_store), ) + full_state = await res.get_state(self._state_controller) + state_resolutions_during_persistence.inc() # If the returned state matches the state group of one of the new @@ -948,7 +950,7 @@ async def _get_new_state_after_events( events_context, ) - return res.state, None, new_latest_event_ids + return full_state, None, new_latest_event_ids async def _prune_extremities( self, From b92429fbe0432293155fa6b61f209f0a2fe175f8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 14 Jul 2022 17:35:19 +0100 Subject: [PATCH 03/11] Add test --- tests/test_state.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/tests/test_state.py b/tests/test_state.py index 6ca8d8f21d66..b2f861a3f6df 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -21,7 +21,7 @@ from synapse.api.room_versions import RoomVersions from synapse.events import make_event_from_dict from synapse.events.snapshot import EventContext -from synapse.state import StateHandler, StateResolutionHandler +from synapse.state import StateHandler, StateResolutionHandler, _make_state_cache_entry from synapse.util import Clock from synapse.util.macaroons import MacaroonGenerator @@ -760,3 +760,32 @@ def _get_context( result = yield defer.ensureDeferred(self.state.compute_event_context(event)) return result + + def test_make_state_cache_entry(self): + "Test that calculating a prev_group and delta is correct" + + new_state = { + ("a", ""): "E", + ("b", ""): "E", + ("c", ""): "E", + } + + # Old state has fewer changes from new state, but the delta involves + # deleting a key, which isn't allowed in the deltas. + old_state_1 = { + ("a", ""): "F", + ("b", ""): "E", + ("c", ""): "E", + ("d", ""): "E", + } + + old_state_2 = { + ("a", ""): "F", + ("b", ""): "F", + ("c", ""): "F", + } + + entry = _make_state_cache_entry(new_state, {1: old_state_1, 2: old_state_2}) + + self.assertEqual(entry.prev_group, 2) + self.assertEqual(entry.delta_ids, new_state) From eb6e0aaac76e0b4629e51595580fd0824ce92d9f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 14 Jul 2022 17:37:56 +0100 Subject: [PATCH 04/11] Newsfile --- changelog.d/13278.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13278.bugfix diff --git a/changelog.d/13278.bugfix b/changelog.d/13278.bugfix new file mode 100644 index 000000000000..7a797d425799 --- /dev/null +++ b/changelog.d/13278.bugfix @@ -0,0 +1 @@ +Fix long-standing bug where in rare instance Synapse could store the incorrect state. From e5e15d800172cdbb42324c552dffdc6931decf4a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 14 Jul 2022 17:49:18 +0100 Subject: [PATCH 05/11] Rename StateCacheEntry.state to reduce confusion --- synapse/state/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index 91c41d8ec4b9..d939d7f7e685 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -84,7 +84,7 @@ def _gen_state_id() -> str: class _StateCacheEntry: - __slots__ = ["state", "state_group", "prev_group", "delta_ids"] + __slots__ = ["_state", "state_group", "prev_group", "delta_ids"] def __init__( self, @@ -97,7 +97,7 @@ def __init__( raise Exception("Either state or state group must be not None") # A map from (type, state_key) to event_id. - self.state = frozendict(state) if state is not None else None + self._state = frozendict(state) if state is not None else None # the ID of a state group if one and only one is involved. # otherwise, None otherwise? @@ -115,8 +115,8 @@ async def get_state( looking up the state group in the DB. """ - if self.state is not None: - return self.state + if self._state is not None: + return self._state assert self.state_group is not None @@ -129,7 +129,7 @@ def __len__(self) -> int: # cache eviction purposes. This is why if `self.state` is None it's fine # to return 1. - return len(self.state) if self.state else 1 + return len(self._state) if self._state else 1 class StateHandler: From deba7e92c4ceaf4bca11fea94d894d9e76f074ee Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 15 Jul 2022 11:14:29 +0100 Subject: [PATCH 06/11] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/13278.bugfix | 2 +- synapse/state/__init__.py | 3 ++- tests/test_state.py | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/changelog.d/13278.bugfix b/changelog.d/13278.bugfix index 7a797d425799..49e9377c7958 100644 --- a/changelog.d/13278.bugfix +++ b/changelog.d/13278.bugfix @@ -1 +1 @@ -Fix long-standing bug where in rare instance Synapse could store the incorrect state. +Fix long-standing bug where in rare instances Synapse could store the incorrect state for a room after a state resolution. diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index d939d7f7e685..52ba0014cb88 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -771,7 +771,8 @@ def _make_state_cache_entry( for old_group, old_state in state_groups_ids.items(): if old_state.keys() - new_state.keys(): # Currently we don't support deltas that remove keys from the state - # map. + # map, so we have to ignore this group as a candidate to base the + # new group on. continue n_delta_ids = {k: v for k, v in new_state.items() if old_state.get(k) != v} diff --git a/tests/test_state.py b/tests/test_state.py index b2f861a3f6df..cb07057b07e2 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -770,8 +770,8 @@ def test_make_state_cache_entry(self): ("c", ""): "E", } - # Old state has fewer changes from new state, but the delta involves - # deleting a key, which isn't allowed in the deltas. + # old_state_1 has fewer differences to new_state than old_state_2, but the delta involves + # deleting a key, which isn't allowed in the deltas, so we should pick old_state_2 as the prev_group. old_state_1 = { ("a", ""): "F", ("b", ""): "E", From 3c5f81ef4c80ee8299e687ba7356cb32b1a86416 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 15 Jul 2022 11:15:55 +0100 Subject: [PATCH 07/11] Rewrap --- tests/test_state.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_state.py b/tests/test_state.py index cb07057b07e2..b0eaebb021a3 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -770,8 +770,9 @@ def test_make_state_cache_entry(self): ("c", ""): "E", } - # old_state_1 has fewer differences to new_state than old_state_2, but the delta involves - # deleting a key, which isn't allowed in the deltas, so we should pick old_state_2 as the prev_group. + # old_state_1 has fewer differences to new_state than old_state_2, but + # the delta involves deleting a key, which isn't allowed in the deltas, + # so we should pick old_state_2 as the prev_group. old_state_1 = { ("a", ""): "F", ("b", ""): "E", From 7a3c1f004aa1ae743a9f7783cfe5ad364c5e588c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 15 Jul 2022 11:16:11 +0100 Subject: [PATCH 08/11] Move full state --- synapse/storage/controllers/persist_events.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/storage/controllers/persist_events.py b/synapse/storage/controllers/persist_events.py index 07c75e86868f..cf98b0ab48f8 100644 --- a/synapse/storage/controllers/persist_events.py +++ b/synapse/storage/controllers/persist_events.py @@ -934,8 +934,6 @@ async def _get_new_state_after_events( state_res_store=StateResolutionStore(self.main_store), ) - full_state = await res.get_state(self._state_controller) - state_resolutions_during_persistence.inc() # If the returned state matches the state group of one of the new @@ -950,6 +948,7 @@ async def _get_new_state_after_events( events_context, ) + full_state = await res.get_state(self._state_controller) return full_state, None, new_latest_event_ids async def _prune_extremities( From 0c9ad7cfa901d2e0a11fc793134cea29243effc9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 15 Jul 2022 11:17:41 +0100 Subject: [PATCH 09/11] Mention when it can be None --- synapse/state/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index 52ba0014cb88..edb6684bc97d 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -97,6 +97,9 @@ def __init__( raise Exception("Either state or state group must be not None") # A map from (type, state_key) to event_id. + # + # This can be None if we have a `state_group` (as then we can fetch the + # state from the DB.) self._state = frozendict(state) if state is not None else None # the ID of a state group if one and only one is involved. From 30d720076b3278ab3b8ec275f13a9e717f1d6d8a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 15 Jul 2022 11:22:07 +0100 Subject: [PATCH 10/11] Better test for delta IDs --- tests/test_state.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/test_state.py b/tests/test_state.py index b0eaebb021a3..10fb2a07fdca 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -768,25 +768,35 @@ def test_make_state_cache_entry(self): ("a", ""): "E", ("b", ""): "E", ("c", ""): "E", + ("d", ""): "E", } # old_state_1 has fewer differences to new_state than old_state_2, but # the delta involves deleting a key, which isn't allowed in the deltas, # so we should pick old_state_2 as the prev_group. + + # `old_state_1` has two differences: `a` and `e` old_state_1 = { ("a", ""): "F", ("b", ""): "E", ("c", ""): "E", ("d", ""): "E", + ("e", ""): "E", } + # `old_state_2` has three differences: `a`, `c` and `d` old_state_2 = { ("a", ""): "F", - ("b", ""): "F", + ("b", ""): "E", ("c", ""): "F", + ("d", ""): "F", } entry = _make_state_cache_entry(new_state, {1: old_state_1, 2: old_state_2}) self.assertEqual(entry.prev_group, 2) - self.assertEqual(entry.delta_ids, new_state) + + # There are two changes from `old_state_2` to `new_state` + self.assertEqual( + entry.delta_ids, {("a", ""): "E", ("c", ""): "E", ("d", ""): "E"} + ) From 1abff219a7a5113dd3236b3e6ea0fdd20694fcb5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 15 Jul 2022 11:35:07 +0100 Subject: [PATCH 11/11] Update tests/test_state.py Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- tests/test_state.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_state.py b/tests/test_state.py index 10fb2a07fdca..e2c0013671c1 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -796,7 +796,7 @@ def test_make_state_cache_entry(self): self.assertEqual(entry.prev_group, 2) - # There are two changes from `old_state_2` to `new_state` + # There are three changes from `old_state_2` to `new_state` self.assertEqual( entry.delta_ids, {("a", ""): "E", ("c", ""): "E", ("d", ""): "E"} )