Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion pkg/storages/fs/folder.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,46 @@ func (folder *Folder) ListFolder() (objects []storage.Object, subFolders []stora

func (folder *Folder) DeleteObjects(objectsWithRelativePaths []storage.Object) error {
for _, object := range objectsWithRelativePaths {
err := os.RemoveAll(folder.GetFilePath(object.GetName()))
filePath := folder.GetFilePath(object.GetName())
err := os.RemoveAll(filePath)
if os.IsNotExist(err) {
continue
}
if err != nil {
return fmt.Errorf("unable to delete file %q: %w", object.GetName(), err)
}
if err = folder.cleanupEmptyFolders(path.Dir(filePath)); err != nil {
return err
}
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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 ..).

Copilot uses AI. Check for mistakes.
}
return nil
}

// cleanupEmptyFolders removes empty parent directories up to the root folder path.
func (folder *Folder) cleanupEmptyFolders(dirPath string) error {
rootPath := path.Clean(folder.rootPath)
for {
dirPath = path.Clean(dirPath)
if dirPath == rootPath || !strings.HasPrefix(dirPath, rootPath) {
break
}
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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 ..).

Copilot uses AI. Check for mistakes.
entries, err := os.ReadDir(dirPath)
if err != nil {
if os.IsNotExist(err) {
break
}
return fmt.Errorf("unable to read directory %q: %w", dirPath, err)
}
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)
}
Comment on lines +90 to +99
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
dirPath = path.Dir(dirPath)
}
return nil
}
Expand Down
61 changes: 61 additions & 0 deletions pkg/storages/fs/folder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package fs
import (
"os"
"path/filepath"
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/wal-g/wal-g/pkg/storages/storage"
Expand All @@ -20,6 +22,65 @@ func TestFSFolder(t *testing.T) {
storage.RunFolderTest(st.RootFolder(), t)
}

func TestDeleteObjectsRemovesEmptyFolders(t *testing.T) {
tmpDir := setupTmpDir(t)
defer os.RemoveAll(tmpDir)

st, err := ConfigureStorage(tmpDir, nil)
assert.NoError(t, err)

root := st.RootFolder()

// Create a nested structure: root/a/b/file
sub := root.GetSubFolder("a/b")
err = sub.PutObject("file", strings.NewReader("data"))
assert.NoError(t, err)

// Verify the directory exists
_, err = os.Stat(filepath.Join(tmpDir, "a", "b"))
assert.NoError(t, err)

// Delete the file
err = sub.DeleteObjects([]storage.Object{storage.NewLocalObject("file", time.Time{}, 0)})
assert.NoError(t, err)

// Both empty subdirectories should have been cleaned up
_, err = os.Stat(filepath.Join(tmpDir, "a", "b"))
assert.True(t, os.IsNotExist(err), "empty subdirectory 'b' should have been removed")

_, err = os.Stat(filepath.Join(tmpDir, "a"))
assert.True(t, os.IsNotExist(err), "empty subdirectory 'a' should have been removed")

// Root directory must still exist
_, err = os.Stat(tmpDir)
assert.NoError(t, err, "root directory must not be removed")
}

func TestDeleteObjectsKeepsNonEmptyFolders(t *testing.T) {
tmpDir := setupTmpDir(t)
defer os.RemoveAll(tmpDir)

st, err := ConfigureStorage(tmpDir, nil)
assert.NoError(t, err)

root := st.RootFolder()

// Create two files in the same subdirectory
sub := root.GetSubFolder("a/b")
err = sub.PutObject("file1", strings.NewReader("data1"))
assert.NoError(t, err)
err = sub.PutObject("file2", strings.NewReader("data2"))
assert.NoError(t, err)

// Delete only one file
err = sub.DeleteObjects([]storage.Object{storage.NewLocalObject("file1", time.Time{}, 0)})
assert.NoError(t, err)

// The subdirectory should still exist because file2 is still there
_, err = os.Stat(filepath.Join(tmpDir, "a", "b"))
assert.NoError(t, err, "non-empty subdirectory 'b' should not be removed")
}

func setupTmpDir(t *testing.T) string {
cwd, err := filepath.Abs("./")
if err != nil {
Expand Down
Loading