Fix a bug which could lead to incorrect state

changelog.d/13278.bugfix

@@ -0,0 +1 @@
+Fix long-standing bug where in rare instance Synapse could store the incorrect state.

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:
@@ -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

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,

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)