RocketChat / RocketChat/Rocket.Chat

Cache coherency issues in structures that bake their reference to avoid lookups

Open
#22,744 3 comments 2 reactions 2 assignees View on GitHub

@sampaiodiego is already working on this.

Since Jul 23, 2021.

Dominant language
TypeScript
Stars
46.1k
Forks
13.9k
Avg merge
3d 3h
Merged PRs (30d)
130

Description

Description:

Since MongoDB is not very good at joins/lookups, projects have to bake caches of the referenced data and update it according to the real data change. But this is impossible to ensure without multi-document atomicity, something mongo 4.x started supporting.

So with time, with downscales, upscales, deploys, crashes and any other downtime.Those caches may get invalidated and never fixed again. One example is rocketchat_room.usersCount. But it's widespread, virtually every cached structure is vulnerable to this problem. This causes usability issues in the long run.

We use a very old Rocket.Chat version, so I'm not asking for support for it, I'm asking that future versions start getting more careful about cache coherency issues. And I'm willing to work together on this groundwork to support mongo multi-document transactions inside Rocket.Chat's infrastructure. I just have no idea on how to start it right now.

Steps to reproduce:

This function: https://github.com/RocketChat/Rocket.Chat/blob/848cc13762431f0ce968f7d766bc599de0d5a065/app/models/server/models/Subscriptions.js#L1241-L1255

If between lines 1246 and 1249 the machine of the server shuts down, the cache will be invalid, forever. Because it only gets incremented/decremented, never actually checked against Subscriptions. The only way to ensure is using mongo multi-document transactions. To make something like this:

const session = client.startSession();
try {
  await session.withTransaction(async () => {
    const result = await Subscriptions.remove({ rid: roomId }, { session });

    if (Match.test(result, Number) && result > 0) {
      await Rooms.incUsersCountById(roomId, - result, { session });
    }

    await Users.removeRoomByRoomId(roomId, { session }); 
  }, {
    readPreference: 'primary',
    readConcern: { level: 'local' },
    writeConcern: { w: 'majority' }
  });
} finally {
  await session.endSession();
}

I have no idea how can this be feasible with Rocket.Chat Meteor models usage, maybe with Raw models, but it's not clear if the change to raw models can be done without impact. Neither how to extend the model API to get this session parameter.

Since it's a race-condition it's very hard to reproduce, we can only explain this with natural language with the distributed systems principles in mind. And maybe running a check for the entire database of other systems, check the subscriptions and the usersCount and how many don't match. We may have a script for that to provide if needed.

A quick win could also be a background task that fixes all the caches, and monitors how this cache breakage is affecting the user. It's not ideal, nor how I would like it fixed, but it can be a good product decision.

I also don't know how deep it affects the usability, if some features depends on this cached data to properly function and are getting broken. We saw that happen in our system, because we check those caches for decision-making, but I really don't have more documented/tangible information to give.

Expected behavior:

Independently of when the shutdown happened, the caches must be coherent otherwise this lookup avoidance hurts usability.

Actual behavior:

Some shutdowns cause cache incoherence through the code, I cited one small example, but there are others.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.