have_seen_event cache is not invalidated when we persist an event

[MadLittleMods]

The have_seen_event cache is not invalidated when we persist an event.

This is evident when a /messages request triggers a /backfill and it claims that we have the same missing state and auth events each time we _process_pulled_event from the backfill response even though we just fetched and persisted those same state and auth events from the previous event we processed.

I see that we have one spot in Synapse where we invalidate the have_seen_event cache (besides when we purge a room) but if I add a log line to _invalidate_caches_for_event, it never runs in my Complement tests. Does this have a bug where the CacheInvalidationWorkerStore stuff never runs in monolith mode?

synapse/synapse/storage/databases/main/cache.py

Lines 211 to 226 in 2b522cc

	def _invalidate_caches_for_event(
	self,
	stream_ordering: int,
	event_id: str,
	room_id: str,
	etype: str,
	state_key: Optional[str],
	redacts: Optional[str],
	relates_to: Optional[str],
	backfilled: bool,
	) -> None:
	# This invalidates any local in-memory cached event objects, the original
	# process triggering the invalidation is responsible for clearing any external
	# cached objects.
	self._invalidate_local_get_event_cache(event_id)
	self.have_seen_event.invalidate((room_id, event_id))

---

Discovered while trying to make Synapse fast enough for this MSC2716 test for importing many batches. As an example, disabling the have_seen_event cache saves 10 seconds for each /messages request in that MSC2716 Complement test because we're not making as many federation requests for /state (speeding up have_seen_event itself is related to #13625)

But this will also make /messages faster in general so we can include it in the faster /messages milestone.

Dev notes

have_seen_event cache originally added in #9953

[DMRobertson]

I see that we have one spot in Synapse where we invalidate the have_seen_event cache (besides when we purge a room) but if I add a log line to _invalidate_caches_for_event, it never runs in my Complement tests. Does this have a bug where the CacheInvalidationWorkerStore stuff never runs in monolith mode?

Seems plausible: they only seem to be called by process_replication_rows and _process_event_stream_row, both of which are for responding to replication messages only.

[DMRobertson]

Not familiar with the event persistence code, but it looks like persist_event and persist_events end up calling _persist_event_batch and then _persist_events_and_state_updates. The latter might be a good place to invalidate caches? But thinking a little more, maybe it should be the place that sends out the replication messages. Not sure where those are.

[clokep]

I wonder if we should be prefilling the cache in this case instead of invalidating it? Isn't it likely that we're going to immediately use that event if we just persisted it?

[DMRobertson]

I had the same thought, but I wasn't sure how easy it is to do that with the cachedList magic.

[MadLittleMods]

Changing the title back to "have_seen_event cache is not invalidated when we persist an event" because even though _invalidate_caches_for_event was called via events over replication (in worker mode), there is another bug in it where we try to invalidate the have_seen_event cache with the wrong key so it still doesn't work, see #13863 (comment)

[MadLittleMods]

Thanks for the input @DMRobertson and @clokep! Really helped ease the pain making the fix in #13863