From c49f7d292e17449b122fa54252661a9a298be9a8 Mon Sep 17 00:00:00 2001
From: Paul Buetow <paul@buetow.org>
Date: Sun, 10 May 2026 08:36:11 +0300
Subject: internal/service: swap GC hard-delete ordering

Remove the media file from disk before deleting the DB row so that
a failed os.Remove doesn't create an orphaned file. If the file is
already missing (os.IsNotExist), proceed with the DB deletion. Other
removal errors abort the iteration for that item.

Also update the existing ordering test to assert removal happens
before HardDeleteMedia, and add a test covering the file-remove-failure
short-circuit.
---
 internal/service/gc.go      | 22 +++++++++++++---------
 internal/service/gc_test.go | 31 ++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 18 deletions(-)

(limited to 'internal')

diff --git a/internal/service/gc.go b/internal/service/gc.go
index 0beecf8..6eb485a 100644
--- a/internal/service/gc.go
+++ b/internal/service/gc.go
@@ -119,6 +119,19 @@ func (w *GCWorker) run(ctx context.Context) {
 			absPath = filepath.Clean(filepath.Join(w.mediaRoot, item.RelPath))
 		}
 
+		if absPath != "" {
+			if err := os.Remove(absPath); err != nil {
+				if os.IsNotExist(err) {
+					// File already gone; safe to proceed with DB deletion.
+				} else {
+					if w.logger != nil {
+						w.logger.Warn("gc remove file", "path", absPath, "err", err)
+					}
+					continue
+				}
+			}
+		}
+
 		if err := w.store.HardDeleteMedia(ctx, item.ID); err != nil {
 			if w.logger != nil {
 				w.logger.Error("gc hard delete", "id", item.ID, "err", err)
@@ -126,15 +139,6 @@ func (w *GCWorker) run(ctx context.Context) {
 			continue
 		}
 
-		if absPath != "" {
-			if err := os.Remove(absPath); err != nil {
-				if w.logger != nil {
-					w.logger.Warn("gc remove file", "path", absPath, "err", err)
-				}
-				continue
-			}
-		}
-
 		if w.logger != nil {
 			w.logger.Info("gc deleted media", "id", item.ID, "path", absPath)
 		}
diff --git a/internal/service/gc_test.go b/internal/service/gc_test.go
index f91d47c..07c8924 100644
--- a/internal/service/gc_test.go
+++ b/internal/service/gc_test.go
@@ -64,7 +64,7 @@ func TestGCWorker_RunOnce(t *testing.T) {
 	}
 }
 
-func TestGCWorker_DBHardDeleteHappensBeforeFileRemoval(t *testing.T) {
+func TestGCWorker_FileRemovedBeforeDBHardDelete(t *testing.T) {
 	now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
 	tmpDir := t.TempDir()
 
@@ -83,8 +83,8 @@ func TestGCWorker_DBHardDeleteHappensBeforeFileRemoval(t *testing.T) {
 				}, nil
 			},
 			HardDeleteMediaFunc: func(ctx context.Context, id int64) error {
-				if _, err := os.Stat(file); err != nil {
-					t.Fatalf("expected file to exist while hard-deleting DB record: %v", err)
+				if _, err := os.Stat(file); !os.IsNotExist(err) {
+					t.Fatalf("expected file to be removed before hard-deleting DB record")
 				}
 				checkedOrdering = true
 				return nil
@@ -99,12 +99,9 @@ func TestGCWorker_DBHardDeleteHappensBeforeFileRemoval(t *testing.T) {
 	if !checkedOrdering {
 		t.Fatal("expected hard delete ordering check to run")
 	}
-	if _, err := os.Stat(file); !os.IsNotExist(err) {
-		t.Fatal("expected file to be removed after DB hard delete succeeds")
-	}
 }
 
-func TestGCWorker_LeavesFileWhenDBHardDeleteFails(t *testing.T) {
+func TestGCWorker_SkipsDBHardDeleteWhenFileRemoveFails(t *testing.T) {
 	now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
 	tmpDir := t.TempDir()
 
@@ -113,7 +110,14 @@ func TestGCWorker_LeavesFileWhenDBHardDeleteFails(t *testing.T) {
 		t.Fatalf("write file: %v", err)
 	}
 
+	// Make directory unwritable so os.Remove fails.
+	if err := os.Chmod(tmpDir, 0o555); err != nil {
+		t.Fatalf("chmod: %v", err)
+	}
+	defer os.Chmod(tmpDir, 0o755)
+
 	deletedAt := now.Add(-8 * 24 * time.Hour)
+	var hardDeleteCalled bool
 	store := &repository.MockStore{
 		MediaRepo: repository.MockMediaRepo{
 			ListDeletedMediaFunc: func(ctx context.Context) ([]model.Media, error) {
@@ -122,7 +126,8 @@ func TestGCWorker_LeavesFileWhenDBHardDeleteFails(t *testing.T) {
 				}, nil
 			},
 			HardDeleteMediaFunc: func(ctx context.Context, id int64) error {
-				return errors.New("db unavailable")
+				hardDeleteCalled = true
+				return nil
 			},
 		},
 	}
@@ -131,8 +136,16 @@ func TestGCWorker_LeavesFileWhenDBHardDeleteFails(t *testing.T) {
 	w := NewGCWorker(store, &clock.MockClock{T: now}, tmpDir, time.Minute, logger).WithAge(7 * 24 * time.Hour)
 	w.RunOnce()
 
+	// Restore permissions so we can verify the file still exists.
+	if err := os.Chmod(tmpDir, 0o755); err != nil {
+		t.Fatalf("restore chmod: %v", err)
+	}
+
+	if hardDeleteCalled {
+		t.Fatal("expected HardDeleteMedia to not be called when file removal fails")
+	}
 	if _, err := os.Stat(file); err != nil {
-		t.Fatalf("expected file to remain when DB hard delete fails: %v", err)
+		t.Fatalf("expected file to remain when file removal fails: %v", err)
 	}
 }
 
-- 
cgit v1.2.3