feat: deleting media when redacted events get removed #225
Conversation
jason-famedly
left a comment
jason-famedly
left a comment
There was a problem hiding this comment.
A few small nit things, a bigger thing and a question. Looking good though
| # Create media worker and it does not run the background tasks | ||
| media_worker = self.make_worker_hs( | ||
| "synapse.app.generic_worker", | ||
| {"worker_name": "media_worker_1", "run_background_tasks": False}, |
There was a problem hiding this comment.
| {"worker_name": "media_worker_1", "run_background_tasks": False}, | |
| {"worker_name": "media_worker_1", "run_background_tasks_on": "master"}, |
The setting should be run_background_tasks_on. It defaults to master if not declared. Let's match what you have up at line 990, explicit is probably better than relying on defaults
There was a problem hiding this comment.
Actually, for both this line and line 990 above, import MAIN_PROCESS_INSTANCE_NAME and assign it as that, then if it ever gets changed from master to main in the future we are covered(something upstream wanted since forever-ago)
| return config | ||
|
|
||
| def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: | ||
| self.profile_handler = self.hs.get_profile_handler() |
There was a problem hiding this comment.
Can lose this, these tests don't use the profile handler
| shorthand=False, | ||
| access_token=self.user_tok, | ||
| ) | ||
| assert channel.code == 404, channel.json_body |
There was a problem hiding this comment.
🤔 This one took me a minute. Of course this is returning a 404. You are requesting media endpoints on the main process instead of the media worker. The main process doesn't have media endpoints loaded, so returns a 404. I bet the response dict has a errcode: M_UNRECOGNIZED bit in it too. Let's try that with the media worker and see what it does
| deleted_media = self.get_success( | ||
| self.hs.get_datastores().main.get_local_media(media_id) | ||
| ) | ||
| assert deleted_media is None, deleted_media |
There was a problem hiding this comment.
Should we be checking if the file got deleted too?
There was a problem hiding this comment.
Pull request overview
This pull request implements automatic deletion of media attached to redacted events when those events are censored after the retention period expires. The feature builds on MSC3911 (restricted media) to track which media is attached to which events, and deletes that media when the corresponding redacted events are permanently censored.
Changes:
- Added database query to retrieve media IDs attached to specific events
- Integrated media deletion into the existing event censorship background task
- Added replication endpoint for media deletion to support multi-worker deployments
- Added comprehensive test coverage for media deletion on redaction censorship
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/storage/test_censor_events.py | New test file covering media deletion scenarios when events are redacted and censored |
| tests/replication/test_multi_media_repo.py | New test case verifying media deletion works correctly in multi-worker setups |
| synapse/storage/databases/main/media_repository.py | Added get_attached_media_ids method to query media by event ID, and cleanup of media_attachments table on remote media deletion |
| synapse/storage/databases/main/censor_events.py | Modified event censorship loop to fetch and delete attached media |
| synapse/media/media_repository.py | Added delete_local_media_ids method to worker class for replication support |
| synapse/replication/http/media.py | New replication servlet to handle media deletion requests across workers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jason-famedly
left a comment
jason-famedly
left a comment
There was a problem hiding this comment.
I think this will do. If there was anything else, we'll fix it up when we find it
itsoyou
force-pushed
the
syk/deletion-on-redaction-censorship
branch
from
9a5c2ff to
ae1c161
Compare
itsoyou
merged commit 9ee17fa
into
jason/media-deletion-redacted-events
Handles the deletion part of the media