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

Media files in url_cache/ and url_cache_thumbnails/ should not be stored in storage providers #10862

Closed
squahtx opened this issue Sep 21, 2021 · 4 comments · Fixed by #10911
Closed
Assignees
Labels
P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@squahtx
Copy link
Contributor

squahtx commented Sep 21, 2021

Description

Spotted by @richvdh:

Currently media files in url_cache/ and url_cache_thumbnails/ get replicated to storage providers.

These files are supposed to be transient, and so should not be stored in any storage providers (eg. S3)

Version information

  • Homeserver: matrix.org
@squahtx squahtx changed the title url_cache and url_cache_thumbnails url_cache/ and url_cache_thumbnails/ should not be stored in storage providers Sep 21, 2021
@squahtx squahtx self-assigned this Sep 21, 2021
@squahtx squahtx changed the title url_cache/ and url_cache_thumbnails/ should not be stored in storage providers Media files in url_cache/ and url_cache_thumbnails/ should not be stored in storage providers Sep 21, 2021
@DMRobertson DMRobertson added S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches labels Sep 21, 2021
@clokep
Copy link
Member

clokep commented Sep 21, 2021

Or I wonder if they're meant to be deleted when we purge old previews?

async def _expire_url_cache_data(self) -> None:
"""Clean up expired url cache content, media and thumbnails."""
# TODO: Delete from backup media store

@squahtx
Copy link
Contributor Author

squahtx commented Sep 23, 2021

Looks like this is a duplicate of #5411, except that issue is about local_thumbnails/ and remote_thumbnail/ as well.
Let's keep this issue for the url_cache part of #5411 and let the original issue be about the thumbnails.

@squahtx
Copy link
Contributor Author

squahtx commented Sep 23, 2021

Or I wonder if they're meant to be deleted when we purge old previews?

Interesting find!

I'd argue that since url cache data is so short lived, it makes sense to not hand it over to any storage providers:

  • Incurring S3/cloud costs for short lived data seems wasteful (echoing the complaint in Synapse uploads thumbnail data to S3 #5411)
  • The impact of omitting it from storage providers acting as backups is limited to 24 hours anyway

... or we could add another per-storage provider config option, defaulting to do-not-store?

@clokep
Copy link
Member

clokep commented Sep 23, 2021

... or we could add another per-storage provider config option, defaulting to do-not-store?

I think limiting config is desirable, so best not to do this unless there's a huge benefit!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants