fix: cleanup orphan thumbnail files on folder deletion - #1450
fix: cleanup orphan thumbnail files on folder deletion#1450KeerthiKumarR wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughFolder deletion now removes image and video thumbnails for selected folders and descendants before deleting database records. Path-prefix queries now match literal backslashes, percent signs, and underscores. ChangesFolder cleanup and path matching
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant delete_folders
participant folder_util_cleanup_thumbnails
participant Filesystem
participant Database
delete_folders->>folder_util_cleanup_thumbnails: requested folder IDs
folder_util_cleanup_thumbnails->>Filesystem: delete image and video thumbnails
Filesystem-->>folder_util_cleanup_thumbnails: deletion result
delete_folders->>Database: delete folder records
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1159 by ensuring thumbnail files on disk are deleted before folder records are removed from the database (where ON DELETE CASCADE would otherwise orphan thumbnail files).
Changes:
- Added
folder_util_cleanup_thumbnails()to delete image/video thumbnail files for deleted folders (and their subfolders). - Wired thumbnail cleanup into both the API folder deletion route and the “obsolete folder” cleanup path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| backend/app/utils/folders.py | Adds thumbnail cleanup helper and invokes it before DB folder deletion in obsolete-folder flow. |
| backend/app/routes/folders.py | Invokes thumbnail cleanup before db_delete_folders_batch in the delete-folders endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/utils/folders.py`:
- Line 35: Update db_get_folder_ids_by_path_prefix to escape literal % and _
characters in the supplied path and add an explicit ESCAPE character to its LIKE
query, preserving descendant-prefix matching. Add a regression test covering
folder names containing both % and _, ensuring lookup and subsequent thumbnail
deletion affect only matching descendants.
- Around line 45-60: The thumbnail cleanup loops in delete_folders and
folder_util_delete_obsolete_folders must not allow database deletion after an
os.remove OSError. Propagate the cleanup failure or otherwise return a failure
state that both callers honor before deleting records; preserve already-missing
files as a benign case when supported.
- Around line 21-65: Add focused tests for folder_util_cleanup_thumbnails
covering image and video thumbnails across descendant folders, literal wildcard
characters in folder paths, missing thumbnail files, and unlink failures. Also
test folder_util_delete_obsolete_folders with the same thumbnail scenarios and
verify folders containing undeleted children are handled correctly, ensuring
both cleanup callers are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40f69c61-746c-4fee-9d32-1a41fd19cb54
📒 Files selected for processing (2)
backend/app/routes/folders.pybackend/app/utils/folders.py
|
@rohan-pandeyy |
|
Address all the coderabbit suggestions |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/database/folders.py (2)
370-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the replacement chain and remove trailing whitespace.
Line 370 exceeds the PEP 8 79-character guideline. Line 371 contains trailing whitespace. Keep the replacement order, but split the expression and leave the blank line empty.
As per path instructions, Python files must follow PEP 8.
Proposed formatting
- escaped_path = root_path.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") - + escaped_path = ( + root_path.replace("\\", "\\\\") + .replace("%", "\\%") + .replace("_", "\\_") + ) +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/folders.py` around lines 370 - 371, Reformat the escaped_path assignment in the surrounding folder-path logic so the existing replacement order remains unchanged while the expression is wrapped within 79 characters. Remove trailing whitespace and ensure the following blank line is truly empty.Source: Path instructions
369-377: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression tests for literal path-prefix matching.
Because Line 370 escapes
\,%, and_before Line 375 appliesLIKE, add automated tests for each character and combinations. Verify that descendant paths match, while wildcard overmatches and sibling prefixes do not. Include thepath + os.sepcall used bybackend/app/utils/folders.py.As per path instructions, critical functionality must have automated, comprehensive tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/folders.py` around lines 369 - 377, Add regression tests for the folder prefix query around the escaped_path handling and its callers in the folders utility, covering literal backslash, percent, underscore, and combinations of these characters. Verify descendant paths match, wildcard-like overmatches and sibling prefixes do not, and include cases using the path + os.sep form.Source: Path instructions
backend/app/utils/folders.py (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a bare
raisewhen re-raisingOSError.
raise eadds the handler’s raise site to the traceback and triggers Ruff TRY201. Keepas efor logging, but replace bothraise estatements withraise.Proposed fix
- raise e + raise ... - raise e + raiseAs per path instructions, use idiomatic Python exception handling.
Also applies to: 64-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/utils/folders.py` around lines 51 - 53, Update the OSError handlers in the thumbnail-removal logic to keep the existing logging with the bound exception and replace both explicit `raise e` statements with bare `raise`, preserving the original traceback and satisfying idiomatic exception handling.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/database/folders.py`:
- Around line 370-371: Reformat the escaped_path assignment in the surrounding
folder-path logic so the existing replacement order remains unchanged while the
expression is wrapped within 79 characters. Remove trailing whitespace and
ensure the following blank line is truly empty.
- Around line 369-377: Add regression tests for the folder prefix query around
the escaped_path handling and its callers in the folders utility, covering
literal backslash, percent, underscore, and combinations of these characters.
Verify descendant paths match, wildcard-like overmatches and sibling prefixes do
not, and include cases using the path + os.sep form.
In `@backend/app/utils/folders.py`:
- Around line 51-53: Update the OSError handlers in the thumbnail-removal logic
to keep the existing logging with the bound exception and replace both explicit
`raise e` statements with bare `raise`, preserving the original traceback and
satisfying idiomatic exception handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1514b520-9cd7-4c41-8eb1-78e145ca1953
📒 Files selected for processing (2)
backend/app/database/folders.pybackend/app/utils/folders.py
Hey @rohan-pandeyy , I've addressed all of CodeRabbit's feedback!
|
Link your account with GitcordThanks for opening this PR, @KeerthiKumarR! To receive Discord notifications and contributor tracking for this organization:
Once linked, Gitcord can notify you about reviews, merges, and more. — Posted by Gitcord |
Fixes #1159
Problem
Currently, when a user deletes a folder or when the sync microservice cleans up an obsolete folder, the application deletes the folder records from the database. Because SQLite enforces
PRAGMA foreign_keys = ON, this correctly cascade-deletes all associated image and video records. However, this database-level cascade does not delete the physical thumbnail files on the disk, permanently leaking storage space whenever a folder is removed.Summary of Changes
This PR introduces a python-level cleanup layer to explicitly remove thumbnail files from the physical disk before executing the database deletion query.
Modified Files
backend/app/utils/folders.py:folder_util_cleanup_thumbnails(folder_ids), which identifies all recursive subfolders, fetches associated images/videos, and usesos.remove()to clean up their thumbnail files.folder_util_delete_obsolete_folders, ensuring the background sync microservice cleans up thumbnails when a folder is deleted from the filesystem.backend/app/routes/folders.py:delete_foldersAPI route, ensuring thumbnails are correctly deleted when a user manually removes folders via the frontend UI.Summary by CodeRabbit