From d67281a176d9059398c4bdbf90d61148a7f76ce0 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Thu, 8 Dec 2022 11:35:49 -0500
Subject: [PATCH] Check the stream position before checking if the cache is
 empty. (#14639)

An empty cache does not mean the entity has no changed, if
it is earlier than the earliest known stream position return that
the entity *has* changed since the cache cannot accurately
answer that query.
---
 changelog.d/14639.bugfix                   | 1 +
 synapse/util/caches/stream_change_cache.py | 9 +++++----
 tests/util/test_stream_change_cache.py     | 7 ++++---
 3 files changed, 10 insertions(+), 7 deletions(-)
 create mode 100644 changelog.d/14639.bugfix

diff --git a/changelog.d/14639.bugfix b/changelog.d/14639.bugfix
new file mode 100644
index 000000000000..8730b10afe7b
--- /dev/null
+++ b/changelog.d/14639.bugfix
@@ -0,0 +1 @@
+Fix a long-standing bug where the user directory and room/user stats might be out of sync.
diff --git a/synapse/util/caches/stream_change_cache.py b/synapse/util/caches/stream_change_cache.py
index c8b17acb59e1..165745954990 100644
--- a/synapse/util/caches/stream_change_cache.py
+++ b/synapse/util/caches/stream_change_cache.py
@@ -213,16 +213,17 @@ def has_any_entity_changed(self, stream_pos: int) -> bool:
         """
         assert isinstance(stream_pos, int)
 
-        if not self._cache:
-            # If the cache is empty, nothing can have changed.
-            return False
-
         # _cache is not valid at or before the earliest known stream position, so
         # return that an entity has changed.
         if stream_pos <= self._earliest_known_stream_pos:
             self.metrics.inc_misses()
             return True
 
+        # If the cache is empty, nothing can have changed.
+        if not self._cache:
+            self.metrics.inc_misses()
+            return False
+
         self.metrics.inc_hits()
         return stream_pos < self._cache.peekitem()[0]
 
diff --git a/tests/util/test_stream_change_cache.py b/tests/util/test_stream_change_cache.py
index 0305741c9940..3df053493b3e 100644
--- a/tests/util/test_stream_change_cache.py
+++ b/tests/util/test_stream_change_cache.py
@@ -144,9 +144,10 @@ def test_has_any_entity_changed(self) -> None:
         """
         cache = StreamChangeCache("#test", 1)
 
-        # With no entities, it returns False for the past, present, and future.
-        self.assertFalse(cache.has_any_entity_changed(0))
-        self.assertFalse(cache.has_any_entity_changed(1))
+        # With no entities, it returns True for the past, present, and False for
+        # the future.
+        self.assertTrue(cache.has_any_entity_changed(0))
+        self.assertTrue(cache.has_any_entity_changed(1))
         self.assertFalse(cache.has_any_entity_changed(2))
 
         # We add an entity