Prevent join->join membership transitions changing member count

[anoadragon453]

Fixes #1294

StatsHandler handles updates to the current_state_delta_stream, and updates room stats such as the amount of state events, joined users, etc.

However, it counts every new join membership as a new user entering a room (and that user being in another room), whereas it's possible for a user's membership status to go from join -> join, for instance when they change their per-room profile information.

This PR adds a check for join->join membership transitions, and bails out early, as none of the further checks are necessary at that point.

Due to this bug, membership stats in many rooms have ended up being wildly larger than their true values. I am not sure if we also want to include a migration step which recalculates these statistics (possibly using the _populate_stats_process_rooms bg update).

Bug introduced in the initial implementation #4338.

[babolivier]

Wouldn't the right fix be to change the if on line 244 into an elif? It seems to me that this is what's causing the bug (i.e. not no-oping if the membership doesn't change).

I'm not sure bailing out early buys us much at this point, but if you're still really keen on doing so it should probably be

if membership == prev_membership:
    continue

instead, and getting rid of the similar conditional clauses lower in the function.

[babolivier]

Oh and also could you add a test please?

[anoadragon453]

Wouldn't the right fix be to change the if on line 244 into an elif?

Oh, yes indeed! I completely missed that. Thanks 🙂

Oh and also could you add a test please?

👍

[anoadragon453]

Seems that running

INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES ('populate_stats_process_rooms', '{}', 'current_state_events_membership');

works to fix the broken room counts, and runs in the background during operation rather than blocking startup.

[anoadragon453]

Applied your fix and added a test @babolivier 🙂 Still not sure what to do w.r.t the background update however. It will likely take a long time on big homeservers, but it does run in the background. I'm tempted to just chuck it in migration step.

[babolivier]

Code lgtm!

Still not sure what to do w.r.t the background update however. It will likely take a long time on big homeservers, but it does run in the background. I'm tempted to just chuck it in migration step.

What exactly are you referring to with "in migration step"? A schema delta?
I'd be in favour of just making homeservers rerun the update, but it's probably better to get other opinions in #synapse-dev first.

[anoadragon453]

Conclusion from #synapse-dev is that we're going to include a delta to re-run the update. However, it's important to note that the populate_stats_process_rooms background update is already set to run if you're upgrading from Synapse <1.0.0.

Additionally, if you've upgraded to v1.18.0 (which doesn't include this fix), the earlier bg job runs, and then update to v1.19.0, you'd end up with only half of your rooms having room stats recalculated after this fix was in place.

So the proposed solution is to switch the old populate_stats_process_rooms background job to a no-op, and then kick off a bg job with a new name, but the same functionality as the old one. This effectively restarts the background job from the beginning, without running it twice in a row, supporting both upgrade usecases.

[anoadragon453]

I've requested the two people involved in the discussion linked above so one can check what I've done matches up.

[richvdh]

lgtm!

I'm not completely sure this is the only bug in the room lists, but it's obviously a big one!

[anoadragon453]

I think the title of the linked bug is a bit misleading, as the discussing therein seems to only discuss the accidental increasing room members.

If this continues to happen via another bug then we can open a new issue.