Safe async event cache

synapse/storage/_base.py

@@ -96,6 +96,10 @@ def _attempt_to_invalidate_cache(
         cache doesn't exist. Mainly used for invalidating caches on workers,
         where they may not have the cache.
 
+        Note that this function does not invalidate any remote caches, only the
+        local in-memory ones. Any remote invalidation must be performed before
+        calling this.
+
         Args:
             cache_name
             key: Entry to invalidate. If None then invalidates the entire
@@ -113,7 +117,7 @@ def _attempt_to_invalidate_cache(
             cache.invalidate_all()
         else:
             # Prefer any local-only invalidation method. Invalidating any non-local
-            # cache beforehand is the responsibility of the worker making the change.
+            # cache must be be done before this.
             invalidate_method = getattr(cache, "invalidate_local", cache.invalidate)
             invalidate_method(tuple(key))
 

synapse/storage/database.py

@@ -801,6 +801,15 @@ async def runInteraction(
         """
 
         async def _run_callbacks(callbacks: List[_CallbackListEntry]):
+            """
+            This function takes a list of mixed sync/async callbacks and executes
+            the async ones first and then the sync callbacks.
+
+            We do this with the assumption that async functions call out to external
+            systems (e.g. to invalidate a cache) and the sync functions make these
+            changes on any local in-memory caches/similar, and thus must be second.
+            """
+
             sync_callbacks: List[_CallbackListEntry] = []
 
             for cb, args, kwargs in callbacks:

synapse/storage/databases/main/events_worker.py

@@ -712,21 +712,41 @@ async def get_missing_events_from_db() -> Dict[str, EventCacheEntry]:
 
         return event_entry_map
 
-    # The next two methods implement get event cache invalidation the first invalidates
-    # the asynchronous cache instance. This may include out-of-process caches such as
-    # Redis/memcache. Once complete we can invalidate any in memory cache in the second
-    # method. The ordering is important here to ensure we don't pull in any remote
-    # invalid value after we invalidate the in-memory cache.
     def invalidate_get_event_cache_after_txn(
         self, txn: LoggingTransaction, event_id: str
     ) -> None:
+        """
+        Prepares a database transaction to invalidate the get event cache for a given
+        event ID when executed successfully. This is achieved by attaching two callbacks
+        to the transaction, one to invalidate the async cache and one for the in memory
+        sync cache (importantly called in that order).
+
+        Arguments:
+            txn: the database transaction to attach the callbacks to
+            event_id: the event ID to be invalidated from caches
+        """
+
         txn.call_after(self._invalidate_async_get_event_cache, event_id)
         txn.call_after(self._invalidate_local_get_event_cache, event_id)
 
     async def _invalidate_async_get_event_cache(self, event_id: str) -> None:
+        """
+        Invalidates an event in the asyncronous get event cache, which may be remote.
+
+        Arguments:
+            event_id: the event ID to invalidate
+        """
+
         await self._get_event_cache.invalidate((event_id,))
 
     def _invalidate_local_get_event_cache(self, event_id: str) -> None:
+        """
+        Invalidates an event in local in-memory get event caches.
+
+        Arguments:
+            event_id: the event ID to invalidate
+        """
+
         self._get_event_cache.invalidate_local((event_id,))
         self._event_ref.pop(event_id, None)
         self._current_event_fetches.pop(event_id, None)