Fix a bug which could lead to incorrect state

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.

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.
@@ -771,7 +774,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}

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(

tests/test_state.py

@@ -768,24 +768,35 @@ def test_make_state_cache_entry(self):
             ("a", ""): "E",
             ("b", ""): "E",
             ("c", ""): "E",
+            ("d", ""): "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` 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"}
+        )