Consider the `origin_server_ts` of the `m.space.child` event when ordering rooms. (#10730)

This updates the ordering of the returned events from the spaces
summary API to that defined in MSC2946 (which updates MSC1772).

Previously a step was skipped causing ordering to be inconsistent with
clients.

3 files changed, 22 insertions, 12 deletions

diff --git a/changelog.d/10730.bugfix b/changelog.d/10730.bugfix
new file mode 100644
index 0000000000..f1612d3c08
--- /dev/null
+++ b/changelog.d/10730.bugfix
@@ -0,0 +1 @@
+Fix a bug where the ordering algorithm was skipping the `origin_server_ts` step in the spaces summary resulting in unstable room orderings.
diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py
index 906985c754..d1b6f3253e 100644
--- a/synapse/handlers/room_summary.py
+++ b/synapse/handlers/room_summary.py
@@ -1139,25 +1139,26 @@ def _is_suggested_child_event(edge_event: EventBase) -> bool:
 _INVALID_ORDER_CHARS_RE = re.compile(r"[^\x20-\x7E]")
 
 
-def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], str]:
+def _child_events_comparison_key(
+    child: EventBase,
+) -> Tuple[bool, Optional[str], int, str]:
     """
     Generate a value for comparing two child events for ordering.
 
-    The rules for ordering are supposed to be:
+    The rules for ordering are:
 
     1. The 'order' key, if it is valid.
-    2. The 'origin_server_ts' of the 'm.room.create' event.
+    2. The 'origin_server_ts' of the 'm.space.child' event.
     3. The 'room_id'.
 
-    But we skip step 2 since we may not have any state from the room.
-
     Args:
         child: The event for generating a comparison key.
 
     Returns:
         The comparison key as a tuple of:
             False if the ordering is valid.
-            The ordering field.
+            The 'order' field or None if it is not given or invalid.
+            The 'origin_server_ts' field.
             The room ID.
     """
     order = child.content.get("order")
@@ -1168,4 +1169,4 @@ def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str],
         order = None
 
     # Items without an order come last.
-    return (order is None, order, child.room_id)
+    return (order is None, order, child.origin_server_ts, child.room_id)
diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py
index ac800afa7d..449ba89e5a 100644
--- a/tests/handlers/test_room_summary.py
+++ b/tests/handlers/test_room_summary.py
@@ -35,10 +35,11 @@ from synapse.types import JsonDict, UserID
 from tests import unittest
 
 
-def _create_event(room_id: str, order: Optional[Any] = None):
-    result = mock.Mock()
+def _create_event(room_id: str, order: Optional[Any] = None, origin_server_ts: int = 0):
+    result = mock.Mock(name=room_id)
     result.room_id = room_id
     result.content = {}
+    result.origin_server_ts = origin_server_ts
     if order is not None:
         result.content["order"] = order
     return result
@@ -63,10 +64,17 @@ class TestSpaceSummarySort(unittest.TestCase):
 
         self.assertEqual([ev2, ev1], _order(ev1, ev2))
 
+    def test_order_origin_server_ts(self):
+        """Origin server  is a tie-breaker for ordering."""
+        ev1 = _create_event("!abc:test", origin_server_ts=10)
+        ev2 = _create_event("!xyz:test", origin_server_ts=30)
+
+        self.assertEqual([ev1, ev2], _order(ev1, ev2))
+
     def test_order_room_id(self):
-        """Room ID is a tie-breaker for ordering."""
-        ev1 = _create_event("!abc:test", "abc")
-        ev2 = _create_event("!xyz:test", "abc")
+        """Room ID is a final tie-breaker for ordering."""
+        ev1 = _create_event("!abc:test")
+        ev2 = _create_event("!xyz:test")
 
         self.assertEqual([ev1, ev2], _order(ev1, ev2))