fs storage: remove empty directories after object deletion#55
Conversation
Co-authored-by: chipitsine <2217296+chipitsine@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Removes empty directory trees left behind when deleting objects from local filesystem storage (WALG_FILE_PREFIX), aligning behavior with object-store semantics.
Changes:
- Add post-delete cleanup that walks up parent directories and removes them if empty (stopping at storage root).
- Add tests asserting empty directories are pruned and non-empty directories are preserved.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/storages/fs/folder.go | Adds empty-parent-directory cleanup after object deletion. |
| pkg/storages/fs/folder_test.go | Adds tests covering directory cleanup behavior after deletions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if err = folder.cleanupEmptyFolders(path.Dir(filePath)); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
path.Dir / path.Clean operate on slash-separated paths (URL paths), not OS filesystem paths. On Windows (and even in some edge cases on Unix when separators differ), this can yield incorrect parent traversal and root comparisons, preventing cleanup or cleaning the wrong directories. Use filepath.Dir and filepath.Clean for filesystem paths, and avoid strings.HasPrefix for path containment checks (it can misclassify /tmp/root2 as within /tmp/root); instead prefer filepath.Rel(rootPath, dirPath) and stop when the rel path is . or starts with .. (or is ..).
| rootPath := path.Clean(folder.rootPath) | ||
| for { | ||
| dirPath = path.Clean(dirPath) | ||
| if dirPath == rootPath || !strings.HasPrefix(dirPath, rootPath) { | ||
| break | ||
| } |
There was a problem hiding this comment.
path.Dir / path.Clean operate on slash-separated paths (URL paths), not OS filesystem paths. On Windows (and even in some edge cases on Unix when separators differ), this can yield incorrect parent traversal and root comparisons, preventing cleanup or cleaning the wrong directories. Use filepath.Dir and filepath.Clean for filesystem paths, and avoid strings.HasPrefix for path containment checks (it can misclassify /tmp/root2 as within /tmp/root); instead prefer filepath.Rel(rootPath, dirPath) and stop when the rel path is . or starts with .. (or is ..).
| if len(entries) > 0 { | ||
| break | ||
| } | ||
| if err = os.Remove(dirPath); err != nil { | ||
| if os.IsNotExist(err) { | ||
| break | ||
| } | ||
| return fmt.Errorf("unable to remove empty directory %q: %w", dirPath, err) | ||
| } |
There was a problem hiding this comment.
There’s a race between os.ReadDir and os.Remove: a directory can become non-empty after the read, causing os.Remove to fail with ENOTEMPTY/EEXIST. Returning an error here can make DeleteObjects fail even though the object deletion succeeded. Treat “directory not empty” removal failures as a normal stop condition (break cleanup) rather than an error, while still erroring on other failure types.
|
@copilot apply changes based on the comments in this thread |
…eanupEmptyFolders Co-authored-by: chipitsine <2217296+chipitsine@users.noreply.github.com>
Applied all three review comments in commit 6e08500:
|
chipitsine
force-pushed
the
master
branch
2 times, most recently
from
3633c51 to
81395bd
Compare
cleanupEmptyFoldershelper topkg/storages/fs/folder.goDeleteObjectsto callcleanupEmptyFoldersafter each file deletionfolder_test.goto verify empty directories are removed after deletionfilepath.Dir/filepath.Clean/filepath.Relinstead ofpath/strings.HasPrefixfor correct OS path handlingOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.