Fix up and add more tests

synapse/handlers/sliding_sync/__init__.py

@@ -1432,12 +1432,6 @@ def _required_state_changes(
         request_state_keys = request_required_state_map.get(event_type, set())
         changed_state_keys = changed_types_to_state_keys.get(event_type, set())
 
-        invalidated_state_keys = (
-            old_state_keys
-            - request_state_keys
-            - {StateValues.WILDCARD, StateValues.LAZY}
-        ) & changed_state_keys
-
         if old_state_keys == request_state_keys:
             # No change to this type
             continue
@@ -1446,9 +1440,20 @@ def _required_state_changes(
             # Nothing *added*, so we skip. Removals happen below.
             continue
 
+        # We only remove state keys from the effective state if they've been
+        # removed from the request *and* the state has changed. This ensures
+        # that if a client removes and then re-adds a state key, we only send
+        # down the associated current state event if its changed (rather than
+        # sending down the same event twice).
+        invalidated_state_keys = (
+            old_state_keys - request_state_keys
+        ) & changed_state_keys
+
         # Always update changes to include the newly added keys
         changes[event_type] = request_state_keys | (
-            old_state_keys - invalidated_state_keys
+            # Wildcard and lazy state keys are not sticky from previous requests
+            (old_state_keys - {StateValues.WILDCARD, StateValues.LAZY})
+            - invalidated_state_keys
         )
 
         if StateValues.WILDCARD in old_state_keys:
@@ -1482,16 +1487,19 @@ def _required_state_changes(
         old_state_keys = prev_required_state_map.get(event_type, set())
         request_state_keys = request_required_state_map.get(event_type, set())
 
-        invalidated_state_keys = (
-            old_state_keys
-            - request_state_keys
-            - {StateValues.WILDCARD, StateValues.LAZY}
-        ) & changed_state_keys
-
         if old_state_keys == request_state_keys:
             # No change.
             continue
 
+        # We only remove state keys from the effective state if they've been
+        # removed from the request *and* the state has changed. This ensures
+        # that if a client removes and then re-adds a state key, we only send
+        # down the associated current state event if its changed (rather than
+        # sending down the same event twice).
+        invalidated_state_keys = (
+            old_state_keys - request_state_keys
+        ) & changed_state_keys
+
         if request_state_keys - old_state_keys:
             # We've expanded the set of state keys, so we just clobber the
             # current set with the new set.
@@ -1501,7 +1509,9 @@ def _required_state_changes(
             # that's a sufficient edge case that we can ignore (as its only a
             # performance optimization).
             changes[event_type] = request_state_keys | (
-                old_state_keys - invalidated_state_keys
+                # Wildcard and lazy state keys are not sticky from previous requests
+                (old_state_keys - {StateValues.WILDCARD, StateValues.LAZY})
+                - invalidated_state_keys
             )
             continue
 

tests/handlers/test_sliding_sync.py

@@ -3835,24 +3835,22 @@ class RequiredStateChangesTestCase(unittest.TestCase):
                     request_required_state_map={},
                     state_deltas={(EventTypes.Member, "@user2:test"): "$event_id"},
                     expected_with_state_deltas=(
-                        # TODO
+                        # Remove `EventTypes.Member` since there's been a change to that
-                        {
+                        # state, (persist the change to required state). That way next
-                            EventTypes.Member: {
+                        # time, they request `EventTypes.Member`, we see that we haven't
-                                "@user3:test",
+                        # sent it before and send the new state. (if we were tracking
-                            }
+                        # that we sent any other state, we should still keep track
-                        },
+                        # that).
+                        {},
                         # We don't need to request anything more if they are requesting
                         # less state now
                         StateFilter.none(),
                     ),
                     expected_without_state_deltas=(
-                        # TODO
+                        # `EventTypes.Member` is no longer requested but since that
-                        {
+                        # state hasn't changed, nothing should change (we should still
-                            EventTypes.Member: {
+                        # keep track that we've sent `EventTypes.Member` before).
-                                "@user2:test",
+                        None,
-                                "@user3:test",
-                            }
-                        },
                         # We don't need to request anything more if they are requesting
                         # less state now
                         StateFilter.none(),
@@ -4012,28 +4010,20 @@ class RequiredStateChangesTestCase(unittest.TestCase):
                     # request. And we need to request all of the state for that type
                     # because we previously, only sent down a few keys.
                     expected_with_state_deltas=(
-                        {"type1": {StateValues.WILDCARD}},
+                        {"type1": {StateValues.WILDCARD, "state_key2", "state_key3"}},
                         StateFilter.from_types([("type1", None)]),
                     ),
-                    # expected_with_state_deltas=(
-                    #     {"type1": {StateValues.WILDCARD, "state_key2", "state_key3"}},
-                    #     StateFilter.from_types([("type1", None)]),
-                    # ),
                     expected_without_state_deltas=(
-                        {"type1": {StateValues.WILDCARD}},
+                        {
+                            "type1": {
+                                StateValues.WILDCARD,
+                                "state_key1",
+                                "state_key2",
+                                "state_key3",
+                            }
+                        },
                         StateFilter.from_types([("type1", None)]),
                     ),
-                    # expected_without_state_deltas=(
-                    #     {
-                    #         "type1": {
-                    #             StateValues.WILDCARD,
-                    #             "state_key1",
-                    #             "state_key2",
-                    #             "state_key3",
-                    #         }
-                    #     },
-                    #     StateFilter.from_types([("type1", None)]),
-                    # ),
                 ),
             ),
         ]
@@ -4057,11 +4047,6 @@ class RequiredStateChangesTestCase(unittest.TestCase):
             test_parameters.expected_without_state_deltas[0],
             "changed_required_state_map does not match (without state_deltas)",
         )
-        logger.info("asdf actual added_state_filter: %s", added_state_filter)
-        logger.info(
-            "asdf expected added_state_filter: %s",
-            test_parameters.expected_with_state_deltas[1],
-        )
         self.assertEqual(
             added_state_filter,
             test_parameters.expected_without_state_deltas[1],