Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring application of bans and ACL #273

Closed
1 of 4 tasks
Gnuxie opened this issue Apr 8, 2022 · 1 comment
Closed
1 of 4 tasks

Refactoring application of bans and ACL #273

Gnuxie opened this issue Apr 8, 2022 · 1 comment
Assignees
@Gnuxie

Comments

@Gnuxie
Copy link
Contributor

Gnuxie commented Apr 8, 2022 •
edited
Loading

#164

Application of bans and ACL is extremely slow with a large number of moderately sized rooms (50+ with a few 20k~ user rooms)

  • We should apply bans and server ACL concurrently to the most recently active rooms first.
    This means rooms where incidents have are taking place get seen to first.
  • Kick users from servers who have been ACL'd because of the unreliability of server ACL. appservice irc already does this so you should be able to copy their approach
  • Revisit the redaction queue and when redaction happens, it is known to be unreliable. Start by cutting out the hack for initial sync and removing from from here.
  • Keep a rolling cache of room state (members, acl) for each protected room that is updated along side the client sync (similar to banlists). This means we can eliminate client api calls to /joined_members and m.room.server_acl in larger rooms, which can save minutes of time for large rooms. We'll want to be smart about members because with a few large rooms this cache can become too big. Lazy members might be worth a look, this isn't an easy problem
@Gnuxie Gnuxie self-assigned this Apr 8, 2022
Gnuxie added a commit that referenced this issue Apr 11, 2022
@Gnuxie
Apply members and server bans to the most recently active rooms first.
a1ea5a4 
#273
Gnuxie added a commit that referenced this issue Apr 11, 2022
@Gnuxie
Apply members and server bans to the most recently active rooms first.
0f67cb6 
#273
Gnuxie added a commit that referenced this issue Apr 11, 2022
@Gnuxie
Apply members and server bans to the most recently active rooms first.
6570e3e 
#273
@Gnuxie Gnuxie mentioned this issue Apr 11, 2022
Merged
Gnuxie added a commit that referenced this issue Apr 29, 2022
@Gnuxie
Revert "New command `!mjolnir since <date or duration> <kick | ban | …
1d29b50 
…show> <limit> [reason] [...rooms]` (#238)"

This reverts commit e05616b.

We are reverting the since command for multiple reasons.

1. The roomMemberTest that this commit brings has an `it` block ("RoomMemberManager counts correctly when we actually join/leave/get banned from the room")
   that now fails in an infinite loop and we struggle to understand how it ever passed.

   > essentially they assert that Mjolnir is in the room here https://github.com/matrix-org/mjolnir/blob/main/test/integration/roomMembersTest.ts#L303
     Which is great, but they've never joined it or have been invited.
     https://github.com/matrix-org/mjolnir/blob/main/test/integration/roomMembersTest.ts#L302 but then we also assert that there's only one person in the room, but this can't be Mjolnir because Mjolnir didn't
     create these rooms https://github.com/matrix-org/mjolnir/blob/main/test/integration/roomMembersTest.ts#L273, yet they're checking that Mjolnir is indeed there.

   The loop in question is also over complicated and hard to read https://github.com/matrix-org/mjolnir/pull/225/files#diff-67e09dce96f975b2df641d3c600f60f48aeeb9e54a472b612bce90535b4aa0e4R285-R295

   Should have been written like this.
   ```
   while(!roomIds.each(roomId => this.mjolnir.protectedRooms[roomId])) {
       await new Promise(resolve => setTimeout(resolve, 1_000));
   }
   ```

2. The RoomMemberManager is broken even after fixing the test (fixing so that it fails "normally" without getting caught in the loop by changing the setup code) and we have decided is not worth fixing yet.
   This is because it is complex & the manager only knows about joins that have happened since Mjolnir has started syncing, this has proven un-initiative to Mjolnir users already.
   Possible solutions are using the planned the full rolling room member/state cache (#273) or using `/messages` with a filter for member events for the `!since` command, since `/messages` is "chronologically" ordered.
Gnuxie added a commit that referenced this issue May 3, 2022
@Gnuxie
Apply members and server bans to the most recently active rooms first. (
c8caf74 
#274)

* Apply members and server bans to the most recently active rooms first.

#273
@ShadowJonathan ShadowJonathan mentioned this issue May 3, 2022
Closed
@turt2live
Copy link
Member

I think the remainder of this is largely #161

@turt2live turt2live closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gnuxie Gnuxie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@turt2live @Gnuxie