Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Purge room when all users have forgotten it #12170

Closed
wants to merge 5 commits into from

Conversation

pingiun
Copy link

@pingiun pingiun commented Mar 5, 2022

Somewhat adresses #4720, based on comment: #4720 (comment)

This will only work for newly forgotten rooms, and only rooms that are completely forgotten by all users. This is not a general fix for people who want to remove rooms after some time and after they have been left by everyone.

Signed-off-by: Jelle Besseling jelle@pingiun.com

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Signed-off-by: Jelle Besseling <jelle@pingiun.com>
@pingiun pingiun requested a review from a team as a code owner March 5, 2022 15:58
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some tests please? Just a couple to check that we don't purge rooms unless everyone has forgotten the room, and one to check that forgetting then rejoining the room works. I'm just very paranoid when it comes to things that delete lots of data 🙂

Thanks for this!

synapse/storage/databases/main/roommember.py Outdated Show resolved Hide resolved
"purge_room_after_last_forget",
self.storage.purge_events.purge_room,
room_id,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an OK first step, but it does mean that if the server gets restarted before the purge is completed it won't be retried.

One option would be to check if we need to purge the room and if so write that to a DB table, and once purged we can remove it from the table.

That can be a separate PR though I think

changelog.d/12170.bugfix Outdated Show resolved Hide resolved
pingiun and others added 2 commits March 9, 2022 11:39
Co-authored-by: Erik Johnston <erikj@jki.re>
Co-authored-by: Erik Johnston <erikj@jki.re>
@pingiun
Copy link
Author

pingiun commented Mar 9, 2022

I think the failing tests are unrelated to my changes? Not sure

@erikjohnston
Copy link
Member

synapse/storage/databases/main/roommember.py:800:18: W291 trailing whitespace

Maybe my suggestion accidentally included a trailing whitespace? Hard to tell on the web IDE

@pingiun
Copy link
Author

pingiun commented Mar 9, 2022

synapse/storage/databases/main/roommember.py:800:18: W291 trailing whitespace

Maybe my suggestion accidentally included a trailing whitespace? Hard to tell on the web IDE

I'll fix it when I add the tests. My previous comment was about other failing tests: https://github.com/matrix-org/synapse/actions/runs/1938567513

@erikjohnston
Copy link
Member

Oh, sorry! Yeah, that looks like a flakey test to me, since its just failed on one run and appears unrelated.

@anoadragon453 anoadragon453 added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 1, 2022
@anoadragon453
Copy link
Member

Hi @pingiun. Are you able to continue working on this? If you need help with writing tests, I'd suggest taking a look at an existing unit test and adapting it.

Awkwardly I'd point you to some existing unit tests for forgetting a room, but it looks like we only have integration tests for that functionality!

I'd instead suggest creating a new testfile in tests/rest/client/ named test_forget.py or similar and crib off an existing test file for the usual unit test structure. We're also open to providing help over in our dev channel if you need any :)

https://matrix.to/#/#synapse-dev:matrix.org

@anoadragon453
Copy link
Member

Closing this PR as it seems to have been abandoned. If you'd like to continue working on this, feel free to re-open / create a new PR. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants