Repository navigation
Race condition where a second join races with cache invalidation over replication and erroneously rejects joining Restricted Room #13185
Description
(This is the cause of a flake in Complement's TestRestrictedRoomsLocalJoin tests when running with workers.)
If room A is a restricted room, restricted to members of room B, then if:
- a user first attempts to join room A and is rejected;
- the user then joins room B successfully; and
- the user then attempts to join room A again
the user can be erroneously rejected from joining room A (in step (3)).
This is because there is a cache, get_rooms_for_user_with_stream_ordering, which is populated in step (1) on the event creator.
In step (2), the event persister will invalidate that cache and send a command over replication for other workers to do the same.
This issue then occurs if the event creator doesn't receive that command until after step (3).
This occurs easily in Complement+workers on CI, where it takes ~200 ms for the invalidation to be received on the new worker (CPU contention in CI is probably playing a big part).
An obvious workaround is for the client to just sleep & retry, but it seems very ugly that we're issuing 403s to clients when they're making fully serial requests.
Another incomplete workaround is for the event creator to invalidate its own cache, but that won't help if the join to room B takes place on a different event creator.
I think a potential solution that would work is to:
- when persisting the join event and sending the invalidation, ensure this is done before finishing the replication request and the client request
- when starting a new join for a restricted room on an event creator, fetch the latest cache invalidation position from Redis and then only start processing the join once replication has caught up past that point.
(I'm not exactly sure how you 'fetch the latest position'; I was rather hoping there'd be SETMAX in Redis but didn't find it. Alternatively, it's probably possible to just PING Redis and treat the PONG as the barrier, but it's not a very nice solution.)
Activity
reivilibre commentedon Jul 7, 2022
Rough conclusions from the discussion:
We have this problem in quite a few areas of Synapse. SyTest's approach here would be to retry in a loop until the 200 comes back — let's do this for now as doing anything more proper is tricky. (It would also be likely to worsen latency if all requests had to wait for things to catch up.)
In real-world situations this is unlikely to be a problem because this would usually propagate in a few milliseconds.
Theoretically this has a slim chance of causing clients to not know the difference between a 'real'/permanent 403 and a temporary one due to the propagation delay, but we're not aware of this happening in practice and this is already a potential problem with other endpoints.
Generally there are bigger issues to solve.
DMRobertson commentedon Jul 7, 2022
In a nutshell, we've written the test expecting "read-after-write consistency" but we don't have that. (I say this because I think it's useful to have the phrase in mind)
MadLittleMods commentedon Oct 6, 2022
👍 Just experienced the stale cache racing while waiting for invalidation from replication with another Complement test but this time it's all within the same
/messagesrequest so it's a little more insidious.Edit: Split off to #14211
De-flake MSC3030 backfill test with Synapse worker replication
De-flake MSC3030 backfill test with Synapse worker replication (#492)