Refactor file deletion (#1954)

* Add file deleter
* Change scene delete code
* Add image/gallery delete code
* Don't remove stash library paths
* Fail silently if file does not exist

pkg/api/resolver_mutation_gallery.go

@@ -5,9 +5,12 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"os"
 	"strconv"
 	"time"
 
+	"github.com/stashapp/stash/pkg/file"
+	"github.com/stashapp/stash/pkg/image"
 	"github.com/stashapp/stash/pkg/manager"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/plugin"
@@ -395,8 +398,14 @@ func (r *mutationResolver) GalleryDestroy(ctx context.Context, input models.Gall
 	}
 
 	var galleries []*models.Gallery
-	var imgsToPostProcess []*models.Image
+	var imgsDestroyed []*models.Image
-	var imgsToDelete []*models.Image
+	fileDeleter := &image.FileDeleter{
+		Deleter: *file.NewDeleter(),
+		Paths:   manager.GetInstance().Paths,
+	}
+
+	deleteGenerated := utils.IsTrue(input.DeleteGenerated)
+	deleteFile := utils.IsTrue(input.DeleteFile)
 
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
 		qb := repo.Gallery()
@@ -422,13 +431,19 @@ func (r *mutationResolver) GalleryDestroy(ctx context.Context, input models.Gall
 				}
 
 				for _, img := range imgs {
-					if err := iqb.Destroy(img.ID); err != nil {
+					if err := image.Destroy(img, iqb, fileDeleter, deleteGenerated, false); err != nil {
 						return err
 					}
 
-					imgsToPostProcess = append(imgsToPostProcess, img)
+					imgsDestroyed = append(imgsDestroyed, img)
 				}
-			} else if input.DeleteFile != nil && *input.DeleteFile {
+
+				if deleteFile {
+					if err := fileDeleter.Files([]string{gallery.Path.String}); err != nil {
+						return err
+					}
+				}
+			} else if deleteFile {
 				// Delete image if it is only attached to this gallery
 				imgs, err := iqb.FindByGalleryID(id)
 				if err != nil {
@@ -442,14 +457,16 @@ func (r *mutationResolver) GalleryDestroy(ctx context.Context, input models.Gall
 					}
 
 					if len(imgGalleries) == 1 {
-						if err := iqb.Destroy(img.ID); err != nil {
+						if err := image.Destroy(img, iqb, fileDeleter, deleteGenerated, deleteFile); err != nil {
 							return err
 						}
 
-						imgsToDelete = append(imgsToDelete, img)
+						imgsDestroyed = append(imgsDestroyed, img)
-						imgsToPostProcess = append(imgsToPostProcess, img)
 					}
 				}
+
+				// we only want to delete a folder-based gallery if it is empty.
+				// don't do this with the file deleter
 			}
 
 			if err := qb.Destroy(id); err != nil {
@@ -459,28 +476,19 @@ func (r *mutationResolver) GalleryDestroy(ctx context.Context, input models.Gall
 
 		return nil
 	}); err != nil {
+		fileDeleter.Rollback()
 		return false, err
 	}
 
-	// if delete file is true, then delete the file as well
+	// perform the post-commit actions
-	// if it fails, just log a message
+	fileDeleter.Commit()
-	if input.DeleteFile != nil && *input.DeleteFile {
-		// #1804 - delete the image files first, since they must be removed
-		// before deleting a folder
-		for _, img := range imgsToDelete {
-			manager.DeleteImageFile(img)
-		}
 
 	for _, gallery := range galleries {
-			manager.DeleteGalleryFile(gallery)
+		// don't delete stash library paths
-		}
+		if utils.IsTrue(input.DeleteFile) && !gallery.Zip && gallery.Path.Valid && !isStashPath(gallery.Path.String) {
-	}
+			// try to remove the folder - it is possible that it is not empty
-
+			// so swallow the error if present
-	// if delete generated is true, then delete the generated files
+			_ = os.Remove(gallery.Path.String)
-	// for the gallery
-	if input.DeleteGenerated != nil && *input.DeleteGenerated {
-		for _, img := range imgsToPostProcess {
-			manager.DeleteGeneratedImageFiles(img)
 		}
 	}
 
@@ -490,13 +498,24 @@ func (r *mutationResolver) GalleryDestroy(ctx context.Context, input models.Gall
 	}
 
 	// call image destroy post hook as well
-	for _, img := range imgsToDelete {
+	for _, img := range imgsDestroyed {
 		r.hookExecutor.ExecutePostHooks(ctx, img.ID, plugin.ImageDestroyPost, nil, nil)
 	}
 
 	return true, nil
 }
 
+func isStashPath(path string) bool {
+	stashConfigs := manager.GetInstance().Config.GetStashPaths()
+	for _, config := range stashConfigs {
+		if path == config.Path {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (r *mutationResolver) AddGalleryImages(ctx context.Context, input models.GalleryAddInput) (bool, error) {
 	galleryID, err := strconv.Atoi(input.GalleryID)
 	if err != nil {

pkg/api/resolver_mutation_image.go

@@ -6,6 +6,8 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/stashapp/stash/pkg/file"
+	"github.com/stashapp/stash/pkg/image"
 	"github.com/stashapp/stash/pkg/manager"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/plugin"
@@ -281,38 +283,34 @@ func (r *mutationResolver) ImageDestroy(ctx context.Context, input models.ImageD
 		return false, err
 	}
 
-	var image *models.Image
+	var i *models.Image
+	fileDeleter := &image.FileDeleter{
+		Deleter: *file.NewDeleter(),
+		Paths:   manager.GetInstance().Paths,
+	}
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
 		qb := repo.Image()
 
-		image, err = qb.Find(imageID)
+		i, err = qb.Find(imageID)
 		if err != nil {
 			return err
 		}
 
-		if image == nil {
+		if i == nil {
 			return fmt.Errorf("image with id %d not found", imageID)
 		}
 
-		return qb.Destroy(imageID)
+		return image.Destroy(i, qb, fileDeleter, utils.IsTrue(input.DeleteGenerated), utils.IsTrue(input.DeleteFile))
 	}); err != nil {
+		fileDeleter.Rollback()
 		return false, err
 	}
 
-	// if delete generated is true, then delete the generated files
+	// perform the post-commit actions
-	// for the image
+	fileDeleter.Commit()
-	if input.DeleteGenerated != nil && *input.DeleteGenerated {
-		manager.DeleteGeneratedImageFiles(image)
-	}
-
-	// if delete file is true, then delete the file as well
-	// if it fails, just log a message
-	if input.DeleteFile != nil && *input.DeleteFile {
-		manager.DeleteImageFile(image)
-	}
 
 	// call post hook after performing the other actions
-	r.hookExecutor.ExecutePostHooks(ctx, image.ID, plugin.ImageDestroyPost, input, nil)
+	r.hookExecutor.ExecutePostHooks(ctx, i.ID, plugin.ImageDestroyPost, input, nil)
 
 	return true, nil
 }
@@ -324,44 +322,41 @@ func (r *mutationResolver) ImagesDestroy(ctx context.Context, input models.Image
 	}
 
 	var images []*models.Image
+	fileDeleter := &image.FileDeleter{
+		Deleter: *file.NewDeleter(),
+		Paths:   manager.GetInstance().Paths,
+	}
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
 		qb := repo.Image()
 
 		for _, imageID := range imageIDs {
 
-			image, err := qb.Find(imageID)
+			i, err := qb.Find(imageID)
 			if err != nil {
 				return err
 			}
 
-			if image == nil {
+			if i == nil {
 				return fmt.Errorf("image with id %d not found", imageID)
 			}
 
-			images = append(images, image)
+			images = append(images, i)
-			if err := qb.Destroy(imageID); err != nil {
+
+			if err := image.Destroy(i, qb, fileDeleter, utils.IsTrue(input.DeleteGenerated), utils.IsTrue(input.DeleteFile)); err != nil {
 				return err
 			}
 		}
 
 		return nil
 	}); err != nil {
+		fileDeleter.Rollback()
 		return false, err
 	}
 
+	// perform the post-commit actions
+	fileDeleter.Commit()
+
 	for _, image := range images {
-		// if delete generated is true, then delete the generated files
-		// for the image
-		if input.DeleteGenerated != nil && *input.DeleteGenerated {
-			manager.DeleteGeneratedImageFiles(image)
-		}
-
-		// if delete file is true, then delete the file as well
-		// if it fails, just log a message
-		if input.DeleteFile != nil && *input.DeleteFile {
-			manager.DeleteImageFile(image)
-		}
-
 		// call post hook after performing the other actions
 		r.hookExecutor.ExecutePostHooks(ctx, image.ID, plugin.ImageDestroyPost, input, nil)
 	}

pkg/api/resolver_mutation_scene.go

@@ -7,6 +7,7 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/stashapp/stash/pkg/file"
 	"github.com/stashapp/stash/pkg/manager"
 	"github.com/stashapp/stash/pkg/manager/config"
 	"github.com/stashapp/stash/pkg/models"
@@ -456,94 +457,93 @@ func (r *mutationResolver) SceneDestroy(ctx context.Context, input models.SceneD
 		return false, err
 	}
 
-	var scene *models.Scene
+	fileNamingAlgo := manager.GetInstance().Config.GetVideoFileNamingAlgorithm()
-	var postCommitFunc func()
+
+	var s *models.Scene
+	fileDeleter := &scene.FileDeleter{
+		Deleter:        *file.NewDeleter(),
+		FileNamingAlgo: fileNamingAlgo,
+		Paths:          manager.GetInstance().Paths,
+	}
+
+	deleteGenerated := utils.IsTrue(input.DeleteGenerated)
+	deleteFile := utils.IsTrue(input.DeleteFile)
+
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
 		qb := repo.Scene()
 		var err error
-		scene, err = qb.Find(sceneID)
+		s, err = qb.Find(sceneID)
 		if err != nil {
 			return err
 		}
 
-		if scene == nil {
+		if s == nil {
 			return fmt.Errorf("scene with id %d not found", sceneID)
 		}
 
-		postCommitFunc, err = manager.DestroyScene(scene, repo)
+		// kill any running encoders
-		return err
+		manager.KillRunningStreams(s, fileNamingAlgo)
+
+		return scene.Destroy(s, repo, fileDeleter, deleteGenerated, deleteFile)
 	}); err != nil {
+		fileDeleter.Rollback()
 		return false, err
 	}
 
 	// perform the post-commit actions
-	postCommitFunc()
+	fileDeleter.Commit()
-
-	// if delete generated is true, then delete the generated files
-	// for the scene
-	if input.DeleteGenerated != nil && *input.DeleteGenerated {
-		manager.DeleteGeneratedSceneFiles(scene, config.GetInstance().GetVideoFileNamingAlgorithm())
-	}
-
-	// if delete file is true, then delete the file as well
-	// if it fails, just log a message
-	if input.DeleteFile != nil && *input.DeleteFile {
-		manager.DeleteSceneFile(scene)
-	}
 
 	// call post hook after performing the other actions
-	r.hookExecutor.ExecutePostHooks(ctx, scene.ID, plugin.SceneDestroyPost, input, nil)
+	r.hookExecutor.ExecutePostHooks(ctx, s.ID, plugin.SceneDestroyPost, input, nil)
 
 	return true, nil
 }
 
 func (r *mutationResolver) ScenesDestroy(ctx context.Context, input models.ScenesDestroyInput) (bool, error) {
 	var scenes []*models.Scene
-	var postCommitFuncs []func()
+	fileNamingAlgo := manager.GetInstance().Config.GetVideoFileNamingAlgorithm()
+
+	fileDeleter := &scene.FileDeleter{
+		Deleter:        *file.NewDeleter(),
+		FileNamingAlgo: fileNamingAlgo,
+		Paths:          manager.GetInstance().Paths,
+	}
+
+	deleteGenerated := utils.IsTrue(input.DeleteGenerated)
+	deleteFile := utils.IsTrue(input.DeleteFile)
+
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
 		qb := repo.Scene()
 
 		for _, id := range input.Ids {
 			sceneID, _ := strconv.Atoi(id)
 
-			scene, err := qb.Find(sceneID)
+			s, err := qb.Find(sceneID)
 			if err != nil {
 				return err
 			}
-			if scene != nil {
+			if s != nil {
-				scenes = append(scenes, scene)
+				scenes = append(scenes, s)
-			}
-			f, err := manager.DestroyScene(scene, repo)
-			if err != nil {
-				return err
 			}
 
-			postCommitFuncs = append(postCommitFuncs, f)
+			// kill any running encoders
+			manager.KillRunningStreams(s, fileNamingAlgo)
+
+			if err := scene.Destroy(s, repo, fileDeleter, deleteGenerated, deleteFile); err != nil {
+				return err
+			}
 		}
 
 		return nil
 	}); err != nil {
+		fileDeleter.Rollback()
 		return false, err
 	}
 
-	for _, f := range postCommitFuncs {
+	// perform the post-commit actions
-		f()
+	fileDeleter.Commit()
-	}
 
-	fileNamingAlgo := config.GetInstance().GetVideoFileNamingAlgorithm()
 	for _, scene := range scenes {
-		// if delete generated is true, then delete the generated files
-		// for the scene
-		if input.DeleteGenerated != nil && *input.DeleteGenerated {
-			manager.DeleteGeneratedSceneFiles(scene, fileNamingAlgo)
-		}
-
-		// if delete file is true, then delete the file as well
-		// if it fails, just log a message
-		if input.DeleteFile != nil && *input.DeleteFile {
-			manager.DeleteSceneFile(scene)
-		}
-
 		// call post hook after performing the other actions
 		r.hookExecutor.ExecutePostHooks(ctx, scene.ID, plugin.SceneDestroyPost, input, nil)
 	}
@@ -646,7 +646,14 @@ func (r *mutationResolver) SceneMarkerDestroy(ctx context.Context, id string) (b
 		return false, err
 	}
 
-	var postCommitFunc func()
+	fileNamingAlgo := manager.GetInstance().Config.GetVideoFileNamingAlgorithm()
+
+	fileDeleter := &scene.FileDeleter{
+		Deleter:        *file.NewDeleter(),
+		FileNamingAlgo: fileNamingAlgo,
+		Paths:          manager.GetInstance().Paths,
+	}
+
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
 		qb := repo.SceneMarker()
 		sqb := repo.Scene()
@@ -661,18 +668,19 @@ func (r *mutationResolver) SceneMarkerDestroy(ctx context.Context, id string) (b
 			return fmt.Errorf("scene marker with id %d not found", markerID)
 		}
 
-		scene, err := sqb.Find(int(marker.SceneID.Int64))
+		s, err := sqb.Find(int(marker.SceneID.Int64))
 		if err != nil {
 			return err
 		}
 
-		postCommitFunc, err = manager.DestroySceneMarker(scene, marker, qb)
+		return scene.DestroyMarker(s, marker, qb, fileDeleter)
-		return err
 	}); err != nil {
+		fileDeleter.Rollback()
 		return false, err
 	}
 
-	postCommitFunc()
+	// perform the post-commit actions
+	fileDeleter.Commit()
 
 	r.hookExecutor.ExecutePostHooks(ctx, markerID, plugin.SceneMarkerDestroyPost, id, nil)
 
@@ -682,7 +690,15 @@ func (r *mutationResolver) SceneMarkerDestroy(ctx context.Context, id string) (b
 func (r *mutationResolver) changeMarker(ctx context.Context, changeType int, changedMarker models.SceneMarker, tagIDs []int) (*models.SceneMarker, error) {
 	var existingMarker *models.SceneMarker
 	var sceneMarker *models.SceneMarker
-	var scene *models.Scene
+	var s *models.Scene
+
+	fileNamingAlgo := manager.GetInstance().Config.GetVideoFileNamingAlgorithm()
+
+	fileDeleter := &scene.FileDeleter{
+		Deleter:        *file.NewDeleter(),
+		FileNamingAlgo: fileNamingAlgo,
+		Paths:          manager.GetInstance().Paths,
+	}
 
 	// Start the transaction and save the scene marker
 	if err := r.withTxn(ctx, func(repo models.Repository) error {
@@ -704,26 +720,31 @@ func (r *mutationResolver) changeMarker(ctx context.Context, changeType int, cha
 				return err
 			}
 
-			scene, err = sqb.Find(int(existingMarker.SceneID.Int64))
+			s, err = sqb.Find(int(existingMarker.SceneID.Int64))
 		}
 		if err != nil {
 			return err
 		}
 
+		// remove the marker preview if the timestamp was changed
+		if s != nil && existingMarker != nil && existingMarker.Seconds != changedMarker.Seconds {
+			seconds := int(existingMarker.Seconds)
+			if err := fileDeleter.MarkMarkerFiles(s, seconds); err != nil {
+				return err
+			}
+		}
+
 		// Save the marker tags
 		// If this tag is the primary tag, then let's not add it.
 		tagIDs = utils.IntExclude(tagIDs, []int{changedMarker.PrimaryTagID})
 		return qb.UpdateTags(sceneMarker.ID, tagIDs)
 	}); err != nil {
+		fileDeleter.Rollback()
 		return nil, err
 	}
 
-	// remove the marker preview if the timestamp was changed
+	// perform the post-commit actions
-	if scene != nil && existingMarker != nil && existingMarker.Seconds != changedMarker.Seconds {
+	fileDeleter.Commit()
-		seconds := int(existingMarker.Seconds)
-		manager.DeleteSceneMarkerFiles(scene, seconds, config.GetInstance().GetVideoFileNamingAlgorithm())
-	}
-
 	return sceneMarker, nil
 }
 

pkg/file/delete.go

@@ -0,0 +1,161 @@
+package file
+
+import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"os"
+
+	"github.com/stashapp/stash/pkg/logger"
+)
+
+const deleteFileSuffix = ".delete"
+
+// RenamerRemover provides access to the Rename and Remove functions.
+type RenamerRemover interface {
+	Rename(oldpath, newpath string) error
+	Remove(name string) error
+	RemoveAll(path string) error
+	Stat(name string) (fs.FileInfo, error)
+}
+
+type renamerRemoverImpl struct {
+	RenameFn    func(oldpath, newpath string) error
+	RemoveFn    func(name string) error
+	RemoveAllFn func(path string) error
+	StatFn      func(path string) (fs.FileInfo, error)
+}
+
+func (r renamerRemoverImpl) Rename(oldpath, newpath string) error {
+	return r.RenameFn(oldpath, newpath)
+}
+
+func (r renamerRemoverImpl) Remove(name string) error {
+	return r.RemoveFn(name)
+}
+
+func (r renamerRemoverImpl) RemoveAll(path string) error {
+	return r.RemoveAllFn(path)
+}
+
+func (r renamerRemoverImpl) Stat(path string) (fs.FileInfo, error) {
+	return r.StatFn(path)
+}
+
+// Deleter is used to safely delete files and directories from the filesystem.
+// During a transaction, files and directories are marked for deletion using
+// the Files and Dirs methods. This will rename the files/directories to be
+// deleted. If the transaction is rolled back, then the files/directories can
+// be restored to their original state with the Abort method. If the
+// transaction is committed, the marked files are then deleted from the
+// filesystem using the Complete method.
+type Deleter struct {
+	RenamerRemover RenamerRemover
+	files          []string
+	dirs           []string
+}
+
+func NewDeleter() *Deleter {
+	return &Deleter{
+		RenamerRemover: renamerRemoverImpl{
+			RenameFn:    os.Rename,
+			RemoveFn:    os.Remove,
+			RemoveAllFn: os.RemoveAll,
+			StatFn:      os.Stat,
+		},
+	}
+}
+
+// Files designates files to be deleted. Each file marked will be renamed to add
+// a `.delete` suffix. An error is returned if a file could not be renamed.
+// Note that if an error is returned, then some files may be left renamed.
+// Abort should be called to restore marked files if this function returns an
+// error.
+func (d *Deleter) Files(paths []string) error {
+	for _, p := range paths {
+		// fail silently if the file does not exist
+		if _, err := d.RenamerRemover.Stat(p); err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				logger.Warnf("File %q does not exist and therefore cannot be deleted. Ignoring.", p)
+				continue
+			}
+
+			return fmt.Errorf("check file %q exists: %w", p, err)
+		}
+
+		if err := d.renameForDelete(p); err != nil {
+			return fmt.Errorf("marking file %q for deletion: %w", p, err)
+		}
+		d.files = append(d.files, p)
+	}
+
+	return nil
+}
+
+// Dirs designates directories to be deleted. Each directory marked will be renamed to add
+// a `.delete` suffix. An error is returned if a directory could not be renamed.
+// Note that if an error is returned, then some directories may be left renamed.
+// Abort should be called to restore marked files/directories if this function returns an
+// error.
+func (d *Deleter) Dirs(paths []string) error {
+	for _, p := range paths {
+		// fail silently if the file does not exist
+		if _, err := d.RenamerRemover.Stat(p); err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				logger.Warnf("Directory %q does not exist and therefore cannot be deleted. Ignoring.", p)
+				continue
+			}
+
+			return fmt.Errorf("check directory %q exists: %w", p, err)
+		}
+
+		if err := d.renameForDelete(p); err != nil {
+			return fmt.Errorf("marking directory %q for deletion: %w", p, err)
+		}
+		d.dirs = append(d.dirs, p)
+	}
+
+	return nil
+}
+
+// Rollback tries to rename all marked files and directories back to their
+// original names and clears the marked list. Any errors encountered are
+// logged. All files will be attempted regardless of any errors occurred.
+func (d *Deleter) Rollback() {
+	for _, f := range append(d.files, d.dirs...) {
+		if err := d.renameForRestore(f); err != nil {
+			logger.Warnf("Error restoring %q: %v", f, err)
+		}
+	}
+
+	d.files = nil
+	d.dirs = nil
+}
+
+// Commit deletes all files marked for deletion and clears the marked list.
+// Any errors encountered are logged. All files will be attempted, regardless
+// of the errors encountered.
+func (d *Deleter) Commit() {
+	for _, f := range d.files {
+		if err := d.RenamerRemover.Remove(f + deleteFileSuffix); err != nil {
+			logger.Warnf("Error deleting file %q: %v", f+deleteFileSuffix, err)
+		}
+	}
+
+	for _, f := range d.dirs {
+		if err := d.RenamerRemover.RemoveAll(f + deleteFileSuffix); err != nil {
+			logger.Warnf("Error deleting directory %q: %v", f+deleteFileSuffix, err)
+		}
+	}
+
+	d.files = nil
+	d.dirs = nil
+}
+
+func (d *Deleter) renameForDelete(path string) error {
+	return d.RenamerRemover.Rename(path, path+deleteFileSuffix)
+}
+
+func (d *Deleter) renameForRestore(path string) error {
+	return d.RenamerRemover.Rename(path+deleteFileSuffix, path)
+}

pkg/image/delete.go

@@ -0,0 +1,48 @@
+package image
+
+import (
+	"github.com/stashapp/stash/pkg/file"
+	"github.com/stashapp/stash/pkg/manager/paths"
+	"github.com/stashapp/stash/pkg/models"
+	"github.com/stashapp/stash/pkg/utils"
+)
+
+type Destroyer interface {
+	Destroy(id int) error
+}
+
+// FileDeleter is an extension of file.Deleter that handles deletion of image files.
+type FileDeleter struct {
+	file.Deleter
+
+	Paths *paths.Paths
+}
+
+// MarkGeneratedFiles marks for deletion the generated files for the provided image.
+func (d *FileDeleter) MarkGeneratedFiles(image *models.Image) error {
+	thumbPath := d.Paths.Generated.GetThumbnailPath(image.Checksum, models.DefaultGthumbWidth)
+	exists, _ := utils.FileExists(thumbPath)
+	if exists {
+		return d.Files([]string{thumbPath})
+	}
+
+	return nil
+}
+
+// Destroy destroys an image, optionally marking the file and generated files for deletion.
+func Destroy(i *models.Image, destroyer Destroyer, fileDeleter *FileDeleter, deleteGenerated, deleteFile bool) error {
+	// don't try to delete if the image is in a zip file
+	if deleteFile && !file.IsZipPath(i.Path) {
+		if err := fileDeleter.Files([]string{i.Path}); err != nil {
+			return err
+		}
+	}
+
+	if deleteGenerated {
+		if err := fileDeleter.MarkGeneratedFiles(i); err != nil {
+			return err
+		}
+	}
+
+	return destroyer.Destroy(i.ID)
+}

pkg/manager/image.go

@@ -2,34 +2,11 @@ package manager
 
 import (
 	"archive/zip"
-	"os"
 	"strings"
 
 	"github.com/stashapp/stash/pkg/logger"
-	"github.com/stashapp/stash/pkg/models"
-	"github.com/stashapp/stash/pkg/utils"
 )
 
-// DeleteGeneratedImageFiles deletes generated files for the provided image.
-func DeleteGeneratedImageFiles(image *models.Image) {
-	thumbPath := GetInstance().Paths.Generated.GetThumbnailPath(image.Checksum, models.DefaultGthumbWidth)
-	exists, _ := utils.FileExists(thumbPath)
-	if exists {
-		err := os.Remove(thumbPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", thumbPath, err.Error())
-		}
-	}
-}
-
-// DeleteImageFile deletes the image file from the filesystem.
-func DeleteImageFile(image *models.Image) {
-	err := os.Remove(image.Path)
-	if err != nil {
-		logger.Warnf("Could not delete file %s: %s", image.Path, err.Error())
-	}
-}
-
 func walkGalleryZip(path string, walkFunc func(file *zip.File) error) error {
 	readCloser, err := zip.OpenReader(path)
 	if err != nil {

pkg/manager/running_streams.go

@@ -44,7 +44,20 @@ func WaitAndDeregisterStream(filepath string, w *http.ResponseWriter, r *http.Re
 	}()
 }
 
-func KillRunningStreams(path string) {
+func KillRunningStreams(scene *models.Scene, fileNamingAlgo models.HashAlgorithm) {
+	killRunningStreams(scene.Path)
+
+	sceneHash := scene.GetHash(fileNamingAlgo)
+
+	if sceneHash == "" {
+		return
+	}
+
+	transcodePath := GetInstance().Paths.Scene.GetTranscodePath(sceneHash)
+	killRunningStreams(transcodePath)
+}
+
+func killRunningStreams(path string) {
 	ffmpeg.KillRunningEncoders(path)
 
 	streamingFilesMutex.RLock()

pkg/manager/scene.go

@@ -2,190 +2,13 @@ package manager
 
 import (
 	"fmt"
-	"os"
-	"path/filepath"
 
 	"github.com/stashapp/stash/pkg/ffmpeg"
-	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/manager/config"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/utils"
 )
 
-// DestroyScene deletes a scene and its associated relationships from the
-// database. Returns a function to perform any post-commit actions.
-func DestroyScene(scene *models.Scene, repo models.Repository) (func(), error) {
-	qb := repo.Scene()
-	mqb := repo.SceneMarker()
-
-	markers, err := mqb.FindBySceneID(scene.ID)
-	if err != nil {
-		return nil, err
-	}
-
-	var funcs []func()
-	for _, m := range markers {
-		f, err := DestroySceneMarker(scene, m, mqb)
-		if err != nil {
-			return nil, err
-		}
-		funcs = append(funcs, f)
-	}
-
-	if err := qb.Destroy(scene.ID); err != nil {
-		return nil, err
-	}
-
-	return func() {
-		for _, f := range funcs {
-			f()
-		}
-	}, nil
-}
-
-// DestroySceneMarker deletes the scene marker from the database and returns a
-// function that removes the generated files, to be executed after the
-// transaction is successfully committed.
-func DestroySceneMarker(scene *models.Scene, sceneMarker *models.SceneMarker, qb models.SceneMarkerWriter) (func(), error) {
-	if err := qb.Destroy(sceneMarker.ID); err != nil {
-		return nil, err
-	}
-
-	// delete the preview for the marker
-	return func() {
-		seconds := int(sceneMarker.Seconds)
-		DeleteSceneMarkerFiles(scene, seconds, config.GetInstance().GetVideoFileNamingAlgorithm())
-	}, nil
-}
-
-// DeleteGeneratedSceneFiles deletes generated files for the provided scene.
-func DeleteGeneratedSceneFiles(scene *models.Scene, fileNamingAlgo models.HashAlgorithm) {
-	sceneHash := scene.GetHash(fileNamingAlgo)
-
-	if sceneHash == "" {
-		return
-	}
-
-	markersFolder := filepath.Join(GetInstance().Paths.Generated.Markers, sceneHash)
-
-	exists, _ := utils.FileExists(markersFolder)
-	if exists {
-		err := os.RemoveAll(markersFolder)
-		if err != nil {
-			logger.Warnf("Could not delete folder %s: %s", markersFolder, err.Error())
-		}
-	}
-
-	thumbPath := GetInstance().Paths.Scene.GetThumbnailScreenshotPath(sceneHash)
-	exists, _ = utils.FileExists(thumbPath)
-	if exists {
-		err := os.Remove(thumbPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", thumbPath, err.Error())
-		}
-	}
-
-	normalPath := GetInstance().Paths.Scene.GetScreenshotPath(sceneHash)
-	exists, _ = utils.FileExists(normalPath)
-	if exists {
-		err := os.Remove(normalPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", normalPath, err.Error())
-		}
-	}
-
-	streamPreviewPath := GetInstance().Paths.Scene.GetStreamPreviewPath(sceneHash)
-	exists, _ = utils.FileExists(streamPreviewPath)
-	if exists {
-		err := os.Remove(streamPreviewPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", streamPreviewPath, err.Error())
-		}
-	}
-
-	streamPreviewImagePath := GetInstance().Paths.Scene.GetStreamPreviewImagePath(sceneHash)
-	exists, _ = utils.FileExists(streamPreviewImagePath)
-	if exists {
-		err := os.Remove(streamPreviewImagePath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", streamPreviewImagePath, err.Error())
-		}
-	}
-
-	transcodePath := GetInstance().Paths.Scene.GetTranscodePath(sceneHash)
-	exists, _ = utils.FileExists(transcodePath)
-	if exists {
-		// kill any running streams
-		KillRunningStreams(transcodePath)
-
-		err := os.Remove(transcodePath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", transcodePath, err.Error())
-		}
-	}
-
-	spritePath := GetInstance().Paths.Scene.GetSpriteImageFilePath(sceneHash)
-	exists, _ = utils.FileExists(spritePath)
-	if exists {
-		err := os.Remove(spritePath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", spritePath, err.Error())
-		}
-	}
-
-	vttPath := GetInstance().Paths.Scene.GetSpriteVttFilePath(sceneHash)
-	exists, _ = utils.FileExists(vttPath)
-	if exists {
-		err := os.Remove(vttPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", vttPath, err.Error())
-		}
-	}
-}
-
-// DeleteSceneMarkerFiles deletes generated files for a scene marker with the
-// provided scene and timestamp.
-func DeleteSceneMarkerFiles(scene *models.Scene, seconds int, fileNamingAlgo models.HashAlgorithm) {
-	videoPath := GetInstance().Paths.SceneMarkers.GetStreamPath(scene.GetHash(fileNamingAlgo), seconds)
-	imagePath := GetInstance().Paths.SceneMarkers.GetStreamPreviewImagePath(scene.GetHash(fileNamingAlgo), seconds)
-	screenshotPath := GetInstance().Paths.SceneMarkers.GetStreamScreenshotPath(scene.GetHash(fileNamingAlgo), seconds)
-
-	exists, _ := utils.FileExists(videoPath)
-	if exists {
-		err := os.Remove(videoPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", videoPath, err.Error())
-		}
-	}
-
-	exists, _ = utils.FileExists(imagePath)
-	if exists {
-		err := os.Remove(imagePath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", imagePath, err.Error())
-		}
-	}
-
-	exists, _ = utils.FileExists(screenshotPath)
-	if exists {
-		err := os.Remove(screenshotPath)
-		if err != nil {
-			logger.Warnf("Could not delete file %s: %s", screenshotPath, err.Error())
-		}
-	}
-}
-
-// DeleteSceneFile deletes the scene video file from the filesystem.
-func DeleteSceneFile(scene *models.Scene) {
-	// kill any running encoders
-	KillRunningStreams(scene.Path)
-
-	err := os.Remove(scene.Path)
-	if err != nil {
-		logger.Warnf("Could not delete file %s: %s", scene.Path, err.Error())
-	}
-}
-
 func GetSceneFileContainer(scene *models.Scene) (ffmpeg.Container, error) {
 	var container ffmpeg.Container
 	if scene.Format.Valid {

pkg/manager/task_clean.go

@@ -3,9 +3,9 @@ package manager
 import (
 	"context"
 	"fmt"
-	"os"
 	"path/filepath"
 
+	"github.com/stashapp/stash/pkg/file"
 	"github.com/stashapp/stash/pkg/image"
 	"github.com/stashapp/stash/pkg/job"
 	"github.com/stashapp/stash/pkg/logger"
@@ -46,7 +46,7 @@ func (j *cleanJob) Execute(ctx context.Context, progress *job.Progress) {
 		if err := j.processImages(ctx, progress, r.Image()); err != nil {
 			return fmt.Errorf("error cleaning images: %w", err)
 		}
-		if err := j.processGalleries(ctx, progress, r.Gallery()); err != nil {
+		if err := j.processGalleries(ctx, progress, r.Gallery(), r.Image()); err != nil {
 			return fmt.Errorf("error cleaning galleries: %w", err)
 		}
 
@@ -146,7 +146,7 @@ func (j *cleanJob) processScenes(ctx context.Context, progress *job.Progress, qb
 	return nil
 }
 
-func (j *cleanJob) processGalleries(ctx context.Context, progress *job.Progress, qb models.GalleryReader) error {
+func (j *cleanJob) processGalleries(ctx context.Context, progress *job.Progress, qb models.GalleryReader, iqb models.ImageReader) error {
 	batchSize := 1000
 
 	findFilter := models.BatchFindFilter(batchSize)
@@ -168,7 +168,7 @@ func (j *cleanJob) processGalleries(ctx context.Context, progress *job.Progress,
 
 		for _, gallery := range galleries {
 			progress.ExecuteTask(fmt.Sprintf("Assessing gallery %s for clean", gallery.GetTitle()), func() {
-				if j.shouldCleanGallery(gallery) {
+				if j.shouldCleanGallery(gallery, iqb) {
 					toDelete = append(toDelete, gallery.ID)
 				} else {
 					// increment progress, no further processing
@@ -308,9 +308,9 @@ func (j *cleanJob) shouldCleanScene(s *models.Scene) bool {
 	return false
 }
 
-func (j *cleanJob) shouldCleanGallery(g *models.Gallery) bool {
+func (j *cleanJob) shouldCleanGallery(g *models.Gallery, qb models.ImageReader) bool {
 	// never clean manually created galleries
-	if !g.Zip {
+	if !g.Path.Valid {
 		return false
 	}
 
@@ -326,20 +326,33 @@ func (j *cleanJob) shouldCleanGallery(g *models.Gallery) bool {
 	}
 
 	config := config.GetInstance()
+	if g.Zip {
 		if !utils.MatchExtension(path, config.GetGalleryExtensions()) {
 			logger.Infof("File extension does not match gallery extensions. Marking to clean: \"%s\"", path)
 			return true
 		}
 
-	if matchFile(path, config.GetImageExcludes()) {
-		logger.Infof("File matched regex. Marking to clean: \"%s\"", path)
-		return true
-	}
-
 		if countImagesInZip(path) == 0 {
 			logger.Infof("Gallery has 0 images. Marking to clean: \"%s\"", path)
 			return true
 		}
+	} else {
+		// folder-based - delete if it has no images
+		count, err := qb.CountByGalleryID(g.ID)
+		if err != nil {
+			logger.Warnf("Error trying to count gallery images for %q: %v", path, err)
+			return false
+		}
+
+		if count == 0 {
+			return true
+		}
+	}
+
+	if matchFile(path, config.GetImageExcludes()) {
+		logger.Infof("File matched regex. Marking to clean: \"%s\"", path)
+		return true
+	}
 
 	return false
 }
@@ -370,26 +383,33 @@ func (j *cleanJob) shouldCleanImage(s *models.Image) bool {
 }
 
 func (j *cleanJob) deleteScene(ctx context.Context, fileNamingAlgorithm models.HashAlgorithm, sceneID int) {
-	var postCommitFunc func()
+	fileNamingAlgo := GetInstance().Config.GetVideoFileNamingAlgorithm()
-	var scene *models.Scene
+
+	fileDeleter := &scene.FileDeleter{
+		Deleter:        *file.NewDeleter(),
+		FileNamingAlgo: fileNamingAlgo,
+		Paths:          GetInstance().Paths,
+	}
+	var s *models.Scene
 	if err := j.txnManager.WithTxn(context.TODO(), func(repo models.Repository) error {
 		qb := repo.Scene()
 
 		var err error
-		scene, err = qb.Find(sceneID)
+		s, err = qb.Find(sceneID)
 		if err != nil {
 			return err
 		}
-		postCommitFunc, err = DestroyScene(scene, repo)
+
-		return err
+		return scene.Destroy(s, repo, fileDeleter, true, false)
 	}); err != nil {
+		fileDeleter.Rollback()
+
 		logger.Errorf("Error deleting scene from database: %s", err.Error())
 		return
 	}
 
-	postCommitFunc()
+	// perform the post-commit actions
-
+	fileDeleter.Commit()
-	DeleteGeneratedSceneFiles(scene, fileNamingAlgorithm)
 
 	GetInstance().PluginCache.ExecutePostHooks(ctx, sceneID, plugin.SceneDestroyPost, nil, nil)
 }
@@ -407,34 +427,33 @@ func (j *cleanJob) deleteGallery(ctx context.Context, galleryID int) {
 }
 
 func (j *cleanJob) deleteImage(ctx context.Context, imageID int) {
-	var checksum string
+	fileDeleter := &image.FileDeleter{
+		Deleter: *file.NewDeleter(),
+		Paths:   GetInstance().Paths,
+	}
 
 	if err := j.txnManager.WithTxn(context.TODO(), func(repo models.Repository) error {
 		qb := repo.Image()
 
-		image, err := qb.Find(imageID)
+		i, err := qb.Find(imageID)
 		if err != nil {
 			return err
 		}
 
-		if image == nil {
+		if i == nil {
 			return fmt.Errorf("image not found: %d", imageID)
 		}
 
-		checksum = image.Checksum
+		return image.Destroy(i, qb, fileDeleter, true, false)
-
-		return qb.Destroy(imageID)
 	}); err != nil {
+		fileDeleter.Rollback()
+
 		logger.Errorf("Error deleting image from database: %s", err.Error())
 		return
 	}
 
-	// remove cache image
+	// perform the post-commit actions
-	pathErr := os.Remove(GetInstance().Paths.Generated.GetThumbnailPath(checksum, models.DefaultGthumbWidth))
+	fileDeleter.Commit()
-	if pathErr != nil {
-		logger.Errorf("Error deleting thumbnail image from cache: %s", pathErr)
-	}
-
 	GetInstance().PluginCache.ExecutePostHooks(ctx, imageID, plugin.ImageDestroyPost, nil, nil)
 }
 

pkg/scene/delete.go

@@ -0,0 +1,158 @@
+package scene
+
+import (
+	"path/filepath"
+
+	"github.com/stashapp/stash/pkg/file"
+	"github.com/stashapp/stash/pkg/manager/paths"
+	"github.com/stashapp/stash/pkg/models"
+	"github.com/stashapp/stash/pkg/utils"
+)
+
+// FileDeleter is an extension of file.Deleter that handles deletion of scene files.
+type FileDeleter struct {
+	file.Deleter
+
+	FileNamingAlgo models.HashAlgorithm
+	Paths          *paths.Paths
+}
+
+// MarkGeneratedFiles marks for deletion the generated files for the provided scene.
+func (d *FileDeleter) MarkGeneratedFiles(scene *models.Scene) error {
+	sceneHash := scene.GetHash(d.FileNamingAlgo)
+
+	if sceneHash == "" {
+		return nil
+	}
+
+	markersFolder := filepath.Join(d.Paths.Generated.Markers, sceneHash)
+
+	exists, _ := utils.FileExists(markersFolder)
+	if exists {
+		if err := d.Dirs([]string{markersFolder}); err != nil {
+			return err
+		}
+	}
+
+	var files []string
+
+	thumbPath := d.Paths.Scene.GetThumbnailScreenshotPath(sceneHash)
+	exists, _ = utils.FileExists(thumbPath)
+	if exists {
+		files = append(files, thumbPath)
+	}
+
+	normalPath := d.Paths.Scene.GetScreenshotPath(sceneHash)
+	exists, _ = utils.FileExists(normalPath)
+	if exists {
+		files = append(files, normalPath)
+	}
+
+	streamPreviewPath := d.Paths.Scene.GetStreamPreviewPath(sceneHash)
+	exists, _ = utils.FileExists(streamPreviewPath)
+	if exists {
+		files = append(files, streamPreviewPath)
+	}
+
+	streamPreviewImagePath := d.Paths.Scene.GetStreamPreviewImagePath(sceneHash)
+	exists, _ = utils.FileExists(streamPreviewImagePath)
+	if exists {
+		files = append(files, streamPreviewImagePath)
+	}
+
+	transcodePath := d.Paths.Scene.GetTranscodePath(sceneHash)
+	exists, _ = utils.FileExists(transcodePath)
+	if exists {
+		files = append(files, transcodePath)
+	}
+
+	spritePath := d.Paths.Scene.GetSpriteImageFilePath(sceneHash)
+	exists, _ = utils.FileExists(spritePath)
+	if exists {
+		files = append(files, spritePath)
+	}
+
+	vttPath := d.Paths.Scene.GetSpriteVttFilePath(sceneHash)
+	exists, _ = utils.FileExists(vttPath)
+	if exists {
+		files = append(files, vttPath)
+	}
+
+	return d.Files(files)
+}
+
+// MarkMarkerFiles deletes generated files for a scene marker with the
+// provided scene and timestamp.
+func (d *FileDeleter) MarkMarkerFiles(scene *models.Scene, seconds int) error {
+	videoPath := d.Paths.SceneMarkers.GetStreamPath(scene.GetHash(d.FileNamingAlgo), seconds)
+	imagePath := d.Paths.SceneMarkers.GetStreamPreviewImagePath(scene.GetHash(d.FileNamingAlgo), seconds)
+	screenshotPath := d.Paths.SceneMarkers.GetStreamScreenshotPath(scene.GetHash(d.FileNamingAlgo), seconds)
+
+	var files []string
+
+	exists, _ := utils.FileExists(videoPath)
+	if exists {
+		files = append(files, videoPath)
+	}
+
+	exists, _ = utils.FileExists(imagePath)
+	if exists {
+		files = append(files, imagePath)
+	}
+
+	exists, _ = utils.FileExists(screenshotPath)
+	if exists {
+		files = append(files, screenshotPath)
+	}
+
+	return d.Files(files)
+}
+
+// Destroy deletes a scene and its associated relationships from the
+// database.
+func Destroy(scene *models.Scene, repo models.Repository, fileDeleter *FileDeleter, deleteGenerated, deleteFile bool) error {
+	qb := repo.Scene()
+	mqb := repo.SceneMarker()
+
+	markers, err := mqb.FindBySceneID(scene.ID)
+	if err != nil {
+		return err
+	}
+
+	for _, m := range markers {
+		if err := DestroyMarker(scene, m, mqb, fileDeleter); err != nil {
+			return err
+		}
+	}
+
+	if deleteFile {
+		if err := fileDeleter.Files([]string{scene.Path}); err != nil {
+			return err
+		}
+	}
+
+	if deleteGenerated {
+		if err := fileDeleter.MarkGeneratedFiles(scene); err != nil {
+			return err
+		}
+	}
+
+	if err := qb.Destroy(scene.ID); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// DestroyMarker deletes the scene marker from the database and returns a
+// function that removes the generated files, to be executed after the
+// transaction is successfully committed.
+func DestroyMarker(scene *models.Scene, sceneMarker *models.SceneMarker, qb models.SceneMarkerWriter, fileDeleter *FileDeleter) error {
+	if err := qb.Destroy(sceneMarker.ID); err != nil {
+		return err
+	}
+
+	// delete the preview for the marker
+	seconds := int(sceneMarker.Seconds)
+	return fileDeleter.MarkMarkerFiles(scene, seconds)
+}

ui/v2.5/src/components/Changelog/versions/v0120.md

@@ -4,10 +4,12 @@
 * Add forward jump 10 second button to video player. ([#1973](https://github.com/stashapp/stash/pull/1973))
 
 ### 🎨 Improvements
+* Rollback operation if files fail to be deleted. ([#1954](https://github.com/stashapp/stash/pull/1954))
 * Prefer right-most Studio match in the file path when autotagging. ([#2057](https://github.com/stashapp/stash/pull/2057))
 * Added plugin hook for Tag merge operation. ([#2010](https://github.com/stashapp/stash/pull/2010))
 
 ### 🐛 Bug fixes
+* Remove empty folder-based galleries during clean. ([#1954](https://github.com/stashapp/stash/pull/1954))
 * Select first scene result in scene tagger where possible. ([#2051](https://github.com/stashapp/stash/pull/2051))
 * Reject dates with invalid format. ([#2052](https://github.com/stashapp/stash/pull/2052))
 * Fix Autostart Video on Play Selected and Continue Playlist default settings not working. ([#2050](https://github.com/stashapp/stash/pull/2050))