Optimise _get_state_after_missing_prev_event: use /state

[richvdh]

If we're missing most of the events in the room state, then we may as well call the /state endpoint, instead of individually requesting each and every event.

Builds on #12013 which added the get_room_state method.

Requires the changes to sytest which landed in matrix-org/sytest#1223, and those which have yet to land in matrix-org/sytest#1231.

[richvdh]

Something odd is going on here.

The tests are - inconsistently - failing on "Can backfill purged history". This seems to affect largely worker-mode deployments.

1. The first thing that is odd is that the worker-mode deployments perform completely differently to monolith deployments; I think this is because we aren't correctly clearing the caches (Error backfilling in room for 10 minutes after purging history #9864) which means it's working on monoliths for the wrong reason. This is not new; it just goes to explain some of the erratic behaviour.

2. The /messages request triggers a backfill, which returns 10 events. It seems that, at least on workers depending on timings, these 10 events might be the test messages, or might include a membership event. It kinda shouldn't matter - we should end up pulling all of them in anyway - but it does mean that we might end up processing things in a different order. Again, this is not new.

3. Possibly related to 2: server-1 appears not to receive some of the test events, and returns 404s for later /state_ids requests for them. I don't really understand why this is possible - even if it misses them, it should backfill them from server-0. That means that server-0 then fails to process some of the events in the /backfill result and ignores them. Once more, I don't think this is new.

4. server-1 ends up persisting some events (in particular, its invite) as outliers. Later, server-0 makes a /state_ids request, which returns an empty list (/_matrix/federation/v1/state returns a 500 error if called for an outlier #12074 (comment)). This is fairly clearly wrong, but again, not new.

5. What is new is that we calculate we have less than 10% of the events in the state (because 0 * 10 >= 0) so then make a /state request, which then returns a 500 (/_matrix/federation/v1/state returns a 500 error if called for an outlier #12074 again). Arguably we could change that >= to a > to work around it, but it kinda misses the point here.

6. If you're unlucky, server-0 ends up blacklisting server-1 due to the 500s, and then the whole test fails. I haven't figured out what causes the blacklist to happen sometimes and not others.

In short, #12074 is the main problem here, though there is a general catalogue of fail.

[richvdh]

Something odd is going on here.

this is all (hopefully) now fixed.

[DMRobertson]

I think you want

Suggested change

	if not await self._store.have_seen_events(room_id, event_id):
	if not await self._store.have_seen_events(room_id, (event_id,)):

Mypy can't spot this because x: str implies x: Iterable[str]. @squahtx noted this pain point a few weeks ago I think.

(Given that CI passed, are we missing some kind of test here?)

[DMRobertson]

Xref: python/typing#256

[richvdh]

Good spot, thank you.

I've fixed this by implementing have_seen_event and using that instead, which seems to have a lower potential for foot-shooting.

I've also added a UT which catches this.

The failure mode here was relatively edge-casey (it requires us to have the event, but not its state), and benign (it resulted in a redundant call to _matrix/federation/event), which was why this wasn't being picked up in the tests before.

[richvdh]

ugh mypy. I'll fix this up another time.

[DMRobertson]

The unit test is a little daunting, but LGTM.

[richvdh]

related: #11249