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.

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

fix(server): don't delete offline files from disk when trash empties #14777

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Dec 18, 2024

Right now, immich deletes files after 30 days that are excluded or outside of import paths, including original files.

This is not intended, if a new exclusion pattern is added or an import path is changed and one or more assets are removed due to it, we should not be deleting original files.

This bug is in my opinion quite serious and could cause data loss.

I can't find the original bug report (github? reddit?)

Edit: This might be it: https://www.reddit.com/r/immich/comments/1gu61r3/immich_tried_to_remove_my_external_libraries_file/

Also adds some e2e tests related to offline files

@etnoy etnoy force-pushed the fix/empty-offline-files branch from 91b3b6a to 0cffb62 Compare December 18, 2024 21:38
@etnoy etnoy force-pushed the fix/empty-offline-files branch 2 times, most recently from 1a6e9bc to e8c5fcf Compare December 18, 2024 22:30
Copy link
Contributor

@zackpollard zackpollard left a comment

Choose a reason for hiding this comment

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

This looks good overall, I think just the point Jason raised, the asset service isn't where that logic should live

@etnoy
Copy link
Contributor Author

etnoy commented Dec 19, 2024

This looks good overall, I think just the point Jason raised, the asset service isn't where that logic should live

The logic still needs to be in the asset service since that's where the deletion is initiated. Ideally I feel like it could be in the trash service, but it isn't for the moment.

@jrasm91 jrasm91 force-pushed the fix/empty-offline-files branch from b6631cd to 4b4d7fc Compare January 7, 2025 18:15
@jrasm91 jrasm91 enabled auto-merge (squash) January 7, 2025 18:21
@jrasm91 jrasm91 merged commit 23f3e73 into main Jan 7, 2025
36 checks passed
@jrasm91 jrasm91 deleted the fix/empty-offline-files branch January 7, 2025 18:25
yosit pushed a commit to yosit/immich that referenced this pull request Jan 13, 2025
…mmich-app#14777)

fix: don't delete offline files from disk when emptying trash

Move logic to asset deletion check
arctic-foxtato pushed a commit to arctic-foxtato/immich that referenced this pull request Jan 14, 2025
…mmich-app#14777)

fix: don't delete offline files from disk when emptying trash

Move logic to asset deletion check
vladd11 pushed a commit to vladd11/immich that referenced this pull request Jan 25, 2025
…mmich-app#14777)

fix: don't delete offline files from disk when emptying trash

Move logic to asset deletion check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants