Introduce thumb.Resolver to decouple browseService from filesystem

browseService.GetThumbnail previously called os.Stat directly, coupling
the service layer to the filesystem and forcing tests to write temporary
files. Introduce a thumb.Resolver interface plus an FSResolver default,
inject it into browseService, and wire NewMediaServiceWithDeps so
production constructs the resolver explicitly. Tests can now swap in a
fake Resolver without touching disk.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

6 files changed, 328 insertions, 44 deletions

diff --git a/player-server/cmd/player/main.go b/player-server/cmd/player/main.go
index 8b80d8d..e1fd069 100644
--- a/player-server/cmd/player/main.go
+++ b/player-server/cmd/player/main.go
@@ -24,22 +24,22 @@ import (
 
 // appDeps bundles all wired service-layer dependencies.
 type appDeps struct {
-	store            repository.Store
-	hasher           auth.Hasher
-	sm               auth.SessionManager
-	cfg              *internal.Config
-	clk              clock.Clock
-	mediaSvc         service.MediaService
-	adminSvc         service.AdminService
-	progressSvc      service.ProgressService
-	authSvc          service.AuthService
-	podcastSvc       service.PodcastEpisodeService
-	playbackHintSvc  service.PlaybackHintsService
-	scanner          scanner.Scanner
-	gcWorker         *service.GCWorker
-	logger           *slog.Logger
-	appCtx           context.Context
-	workersStarted   chan<- struct{}
+	store           repository.Store
+	hasher          auth.Hasher
+	sm              auth.SessionManager
+	cfg             *internal.Config
+	clk             clock.Clock
+	mediaSvc        service.MediaService
+	adminSvc        service.AdminService
+	progressSvc     service.ProgressService
+	authSvc         service.AuthService
+	podcastSvc      service.PodcastEpisodeService
+	playbackHintSvc service.PlaybackHintsService
+	scanner         scanner.Scanner
+	gcWorker        *service.GCWorker
+	logger          *slog.Logger
+	appCtx          context.Context
+	workersStarted  chan<- struct{}
 }
 
 // parseVersionFlag parses CLI flags and returns whether --version was requested.
@@ -90,10 +90,14 @@ func wireDeps(cfg *internal.Config, store repository.Store, logger *slog.Logger,
 
 	prober := probe.NewFFProber()
 	thumbGen := thumb.NewFFmpegGenerator()
+	// Explicit filesystem thumbnail resolver: keeps service.GetThumbnail
+	// free of direct os.Stat calls and makes the dependency easy to swap
+	// out in tests or alternate deployments (e.g. object storage).
+	thumbResolver := thumb.NewFSResolver()
 
 	helper := service.NewAccessHelper(store)
 	browser := service.NewPodcastBrowseService(store, cfg.MediaRoot)
-	mediaSvc := service.NewMediaServiceWithPodcastBrowser(store, clk, cfg.MediaRoot, thumbGen, prober, browser)
+	mediaSvc := service.NewMediaServiceWithDeps(store, clk, cfg.MediaRoot, thumbGen, prober, browser, thumbResolver)
 	playbackHintSvc := service.NewPlaybackHintsService(helper)
 
 	fsScanner := scanner.NewFSScannerWithLogger(store, prober, thumbGen, clk, cfg.MediaRoot, logger)
diff --git a/player-server/internal/service/browse.go b/player-server/internal/service/browse.go
index 0294a91..be0ee2c 100644
--- a/player-server/internal/service/browse.go
+++ b/player-server/internal/service/browse.go
@@ -2,6 +2,7 @@ package service
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	mrand "math/rand"
 	"os"
@@ -12,15 +13,21 @@ import (
 	"codeberg.org/snonux/player/internal/clock"
 	"codeberg.org/snonux/player/internal/model"
 	"codeberg.org/snonux/player/internal/repository"
+	"codeberg.org/snonux/player/internal/thumb"
 )
 
 // browseService handles read-only browsing and media streaming operations.
+//
+// The thumbnail resolver is injected so the service layer no longer reaches
+// into the filesystem itself; tests can swap in a fake Resolver instead of
+// writing temporary files.
 type browseService struct {
 	store          repository.BrowseServiceStore
 	clock          clock.Clock
 	mediaRoot      string
 	helper         *accessHelper
 	podcastBrowser PodcastBrowser
+	thumbResolver  thumb.Resolver
 }
 
 // PodcastBrowser augments a BrowseResult with podcast-specific folders and episodes.
@@ -75,14 +82,27 @@ func (p *podcastBrowseService) AugmentBrowseSet(ctx context.Context, result *Bro
 	return nil
 }
 
-// NewBrowseService creates a BrowseService.
+// NewBrowseService creates a BrowseService with the production filesystem
+// thumbnail resolver. Use NewBrowseServiceWithResolver to inject a custom
+// Resolver (e.g. a fake in tests).
 func NewBrowseService(store repository.BrowseServiceStore, clk clock.Clock, mediaRoot string, helper *accessHelper, browser PodcastBrowser) *browseService {
+	return NewBrowseServiceWithResolver(store, clk, mediaRoot, helper, browser, thumb.NewFSResolver())
+}
+
+// NewBrowseServiceWithResolver creates a BrowseService with a caller-supplied
+// thumbnail Resolver. A nil resolver falls back to the default filesystem
+// implementation so existing callers keep working.
+func NewBrowseServiceWithResolver(store repository.BrowseServiceStore, clk clock.Clock, mediaRoot string, helper *accessHelper, browser PodcastBrowser, resolver thumb.Resolver) *browseService {
+	if resolver == nil {
+		resolver = thumb.NewFSResolver()
+	}
 	return &browseService{
 		store:          store,
 		clock:          clk,
 		mediaRoot:      mediaRoot,
 		helper:         helper,
 		podcastBrowser: browser,
+		thumbResolver:  resolver,
 	}
 }
 
@@ -229,32 +249,22 @@ func (s *browseService) GetThumbnail(ctx context.Context, mediaID, userID int64)
 	if err != nil {
 		return nil, err
 	}
-	if media.ThumbnailPath == "" {
-		// Use the sentinel so handleError maps this to HTTP 404 instead of
-		// falling through to the default 500 branch.
-		return nil, ErrNotFound
-	}
-	info, err := os.Stat(media.ThumbnailPath)
-	if err == nil {
-		return &FileResult{
-			Path:     media.ThumbnailPath,
-			FileName: filepath.Base(media.ThumbnailPath),
-			FileSize: info.Size(),
-		}, nil
-	}
-	// If the generated thumbnail is missing, fall back to the original file
-	// for image media so that cover.jpg and similar files still render.
-	if media.Type == model.MediaTypeImage {
-		info, err = os.Stat(media.AbsPath)
-		if err == nil {
-			return &FileResult{
-				Path:     media.AbsPath,
-				FileName: filepath.Base(media.AbsPath),
-				FileSize: info.Size(),
-			}, nil
+	// Delegate filesystem access to the injected resolver so the service
+	// layer stays free of os.Stat. The resolver returns thumb.ErrNotFound
+	// for missing thumbnails; map that to the service-level sentinel so
+	// handleError renders an HTTP 404 instead of a 500.
+	resolved, err := s.thumbResolver.Resolve(media)
+	if err != nil {
+		if errors.Is(err, thumb.ErrNotFound) {
+			return nil, ErrNotFound
 		}
+		return nil, err
 	}
-	return nil, fmt.Errorf("stat thumbnail: %w", err)
+	return &FileResult{
+		Path:     resolved.Path,
+		FileName: resolved.FileName,
+		FileSize: resolved.FileSize,
+	}, nil
 }
 
 // prefixForParent builds the slash-terminated prefix used for matching paths under parent.
diff --git a/player-server/internal/service/browse_test.go b/player-server/internal/service/browse_test.go
index 81ffde1..655bfe5 100644
--- a/player-server/internal/service/browse_test.go
+++ b/player-server/internal/service/browse_test.go
@@ -8,8 +8,24 @@ import (
 	"codeberg.org/snonux/player/internal/clock"
 	"codeberg.org/snonux/player/internal/model"
 	"codeberg.org/snonux/player/internal/repository"
+	"codeberg.org/snonux/player/internal/thumb"
 )
 
+// fakeResolver is a test double for thumb.Resolver that lets tests assert
+// browseService delegates thumbnail lookup instead of touching the disk.
+type fakeResolver struct {
+	called    bool
+	gotMedia  *model.Media
+	resolved  *thumb.ResolvedFile
+	resolvErr error
+}
+
+func (f *fakeResolver) Resolve(media *model.Media) (*thumb.ResolvedFile, error) {
+	f.called = true
+	f.gotMedia = media
+	return f.resolved, f.resolvErr
+}
+
 func TestBrowseService_BrowseSet(t *testing.T) {
 	ctx := context.Background()
 	tmpDir := t.TempDir()
@@ -177,3 +193,70 @@ func TestBrowseService_BrowseSet_PodcastEpisodes(t *testing.T) {
 		t.Fatalf("expected undownloaded episode ID 2, got %+v", res.Episodes[0])
 	}
 }
+
+// TestBrowseService_GetThumbnail_DelegatesToResolver verifies the service
+// no longer calls os.Stat directly: GetThumbnail should hand off to the
+// injected thumb.Resolver and translate ResolvedFile -> FileResult.
+func TestBrowseService_GetThumbnail_DelegatesToResolver(t *testing.T) {
+	ctx := context.Background()
+
+	media := &model.Media{ID: 7, SetID: 1, FileName: "a.mp4", ThumbnailPath: "/anywhere/x.jpg"}
+	store := &repository.MockStore{
+		MediaRepo: repository.MockMediaRepo{
+			GetMediaByIDFunc: func(ctx context.Context, id int64) (*model.Media, error) {
+				return media, nil
+			},
+		},
+		UserRepo: repository.MockUserRepo{
+			GetUserByIDFunc: func(ctx context.Context, id int64) (*model.User, error) {
+				return &model.User{ID: id, IsAdmin: true}, nil
+			},
+		},
+	}
+
+	t.Run("resolver hit becomes FileResult", func(t *testing.T) {
+		fake := &fakeResolver{resolved: &thumb.ResolvedFile{Path: "/anywhere/x.jpg", FileName: "x.jpg", FileSize: 42}}
+		svc := NewBrowseServiceWithResolver(store, clock.RealClock{}, "/tmp/media", &accessHelper{store: store}, nil, fake)
+		res, err := svc.GetThumbnail(ctx, 7, 1)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if !fake.called {
+			t.Fatal("expected resolver to be invoked")
+		}
+		if fake.gotMedia == nil || fake.gotMedia.ID != 7 {
+			t.Fatalf("resolver received wrong media: %+v", fake.gotMedia)
+		}
+		if res.Path != "/anywhere/x.jpg" || res.FileName != "x.jpg" || res.FileSize != 42 {
+			t.Fatalf("unexpected FileResult: %+v", res)
+		}
+	})
+
+	t.Run("resolver ErrNotFound maps to service ErrNotFound", func(t *testing.T) {
+		fake := &fakeResolver{resolvErr: thumb.ErrNotFound}
+		svc := NewBrowseServiceWithResolver(store, clock.RealClock{}, "/tmp/media", &accessHelper{store: store}, nil, fake)
+		_, err := svc.GetThumbnail(ctx, 7, 1)
+		if !errors.Is(err, ErrNotFound) {
+			t.Fatalf("expected ErrNotFound, got %v", err)
+		}
+	})
+
+	t.Run("other resolver errors propagate", func(t *testing.T) {
+		boom := errors.New("disk on fire")
+		fake := &fakeResolver{resolvErr: boom}
+		svc := NewBrowseServiceWithResolver(store, clock.RealClock{}, "/tmp/media", &accessHelper{store: store}, nil, fake)
+		_, err := svc.GetThumbnail(ctx, 7, 1)
+		if err == nil || errors.Is(err, ErrNotFound) {
+			t.Fatalf("expected non-not-found error, got %v", err)
+		}
+	})
+
+	t.Run("nil resolver falls back to default", func(t *testing.T) {
+		// NewBrowseServiceWithResolver(nil resolver) should still work and
+		// not panic — the constructor swaps in the FS resolver.
+		svc := NewBrowseServiceWithResolver(store, clock.RealClock{}, "/tmp/media", &accessHelper{store: store}, nil, nil)
+		if svc == nil {
+			t.Fatal("expected non-nil service")
+		}
+	})
+}
diff --git a/player-server/internal/service/media.go b/player-server/internal/service/media.go
index 38c670f..2640d0c 100644
--- a/player-server/internal/service/media.go
+++ b/player-server/internal/service/media.go
@@ -33,11 +33,21 @@ func NewMediaService(store repository.MediaServiceStore, clk clock.Clock, mediaR
 	return NewMediaServiceWithPodcastBrowser(store, clk, mediaRoot, thumbGen, prober, nil)
 }
 
-// NewMediaServiceWithPodcastBrowser creates a MediaService with an optional PodcastBrowser.
+// NewMediaServiceWithPodcastBrowser creates a MediaService with an optional
+// PodcastBrowser and the default filesystem thumbnail resolver. For
+// dependency-injected setups (e.g. tests that want to avoid touching disk
+// for thumbnails) use NewMediaServiceWithDeps.
 func NewMediaServiceWithPodcastBrowser(store repository.MediaServiceStore, clk clock.Clock, mediaRoot string, thumbGen thumb.Generator, prober probe.Prober, browser PodcastBrowser) *mediaService {
+	return NewMediaServiceWithDeps(store, clk, mediaRoot, thumbGen, prober, browser, thumb.NewFSResolver())
+}
+
+// NewMediaServiceWithDeps creates a MediaService with all collaborators
+// supplied explicitly, including a thumbnail Resolver. A nil resolver
+// falls back to the default filesystem implementation.
+func NewMediaServiceWithDeps(store repository.MediaServiceStore, clk clock.Clock, mediaRoot string, thumbGen thumb.Generator, prober probe.Prober, browser PodcastBrowser, thumbResolver thumb.Resolver) *mediaService {
 	helper := &accessHelper{store: store}
 	return &mediaService{
-		MediaBrowseService:   NewBrowseService(store, clk, mediaRoot, helper, browser),
+		MediaBrowseService:   NewBrowseServiceWithResolver(store, clk, mediaRoot, helper, browser, thumbResolver),
 		MediaWriteService:    NewWriteService(store, clk, mediaRoot, thumbGen, prober, helper),
 		MediaShareService:    NewShareService(store, clk, helper),
 		MediaTagService:      NewTagService(store, helper),
diff --git a/player-server/internal/thumb/resolver.go b/player-server/internal/thumb/resolver.go
new file mode 100644
index 0000000..a6614d6
--- /dev/null
+++ b/player-server/internal/thumb/resolver.go
@@ -0,0 +1,80 @@
+package thumb
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+
+	"codeberg.org/snonux/player/internal/model"
+)
+
+// ErrNotFound is returned by a Resolver when a media item has no thumbnail
+// (and no acceptable fallback) available. Callers translate this sentinel
+// to their domain-specific not-found error (e.g. service.ErrNotFound).
+var ErrNotFound = errors.New("thumbnail not found")
+
+// ResolvedFile describes a thumbnail file that has been located on (or
+// abstractly resolved from) some backing store. It contains everything a
+// caller needs to construct a HTTP file response without itself touching
+// the filesystem.
+type ResolvedFile struct {
+	Path     string
+	FileName string
+	FileSize int64
+}
+
+// Resolver abstracts the "given a media item, return a thumbnail file" step.
+// Pulling this out of browseService lets the service layer avoid direct
+// os.Stat calls and lets tests inject a fake resolver instead of writing
+// temporary files.
+type Resolver interface {
+	// Resolve returns a ResolvedFile for the media's thumbnail. If the
+	// media has no thumbnail and no fallback is available, it returns
+	// ErrNotFound. Any other error indicates a real I/O problem.
+	Resolve(media *model.Media) (*ResolvedFile, error)
+}
+
+// FSResolver is the production Resolver that stats files on the local
+// filesystem. It encapsulates the original logic that lived in
+// browseService.GetThumbnail: prefer the generated thumbnail, fall back to
+// the original file for images so that cover.jpg-style assets still render.
+type FSResolver struct{}
+
+// NewFSResolver returns a Resolver backed by os.Stat against real paths.
+func NewFSResolver() *FSResolver {
+	return &FSResolver{}
+}
+
+// Resolve looks up the thumbnail file on disk for media, falling back to
+// the original AbsPath for image media when the generated thumbnail is
+// missing. A missing or empty thumbnail path with no fallback yields
+// ErrNotFound so callers can map it to a 404 cleanly.
+func (FSResolver) Resolve(media *model.Media) (*ResolvedFile, error) {
+	if media == nil {
+		return nil, ErrNotFound
+	}
+	if media.ThumbnailPath == "" {
+		return nil, ErrNotFound
+	}
+	if info, err := os.Stat(media.ThumbnailPath); err == nil {
+		return &ResolvedFile{
+			Path:     media.ThumbnailPath,
+			FileName: filepath.Base(media.ThumbnailPath),
+			FileSize: info.Size(),
+		}, nil
+	} else if media.Type == model.MediaTypeImage {
+		// Generated thumbnail missing: for images, fall back to the
+		// original file so cover.jpg / folder.jpg etc. still render.
+		if info, statErr := os.Stat(media.AbsPath); statErr == nil {
+			return &ResolvedFile{
+				Path:     media.AbsPath,
+				FileName: filepath.Base(media.AbsPath),
+				FileSize: info.Size(),
+			}, nil
+		}
+		return nil, fmt.Errorf("stat thumbnail: %w", err)
+	} else {
+		return nil, fmt.Errorf("stat thumbnail: %w", err)
+	}
+}
diff --git a/player-server/internal/thumb/resolver_test.go b/player-server/internal/thumb/resolver_test.go
new file mode 100644
index 0000000..abf9071
--- /dev/null
+++ b/player-server/internal/thumb/resolver_test.go
@@ -0,0 +1,97 @@
+package thumb
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"codeberg.org/snonux/player/internal/model"
+)
+
+func TestFSResolver_Resolve(t *testing.T) {
+	tmpDir := t.TempDir()
+	thumbPath := filepath.Join(tmpDir, "thumb.jpg")
+	if err := os.WriteFile(thumbPath, []byte("thumb-bytes"), 0o644); err != nil {
+		t.Fatalf("write thumb: %v", err)
+	}
+	imgPath := filepath.Join(tmpDir, "cover.jpg")
+	if err := os.WriteFile(imgPath, []byte("imgcontents"), 0o644); err != nil {
+		t.Fatalf("write img: %v", err)
+	}
+
+	r := NewFSResolver()
+
+	t.Run("nil media is not found", func(t *testing.T) {
+		if _, err := r.Resolve(nil); !errors.Is(err, ErrNotFound) {
+			t.Fatalf("expected ErrNotFound, got %v", err)
+		}
+	})
+
+	t.Run("empty thumbnail path is not found", func(t *testing.T) {
+		_, err := r.Resolve(&model.Media{ID: 1})
+		if !errors.Is(err, ErrNotFound) {
+			t.Fatalf("expected ErrNotFound, got %v", err)
+		}
+	})
+
+	t.Run("existing thumbnail returns resolved file", func(t *testing.T) {
+		res, err := r.Resolve(&model.Media{ID: 1, ThumbnailPath: thumbPath})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if res.Path != thumbPath {
+			t.Fatalf("path = %q, want %q", res.Path, thumbPath)
+		}
+		if res.FileName != "thumb.jpg" {
+			t.Fatalf("file name = %q", res.FileName)
+		}
+		if res.FileSize != int64(len("thumb-bytes")) {
+			t.Fatalf("file size = %d", res.FileSize)
+		}
+	})
+
+	t.Run("image falls back to AbsPath when thumb missing", func(t *testing.T) {
+		missing := filepath.Join(tmpDir, "missing.jpg")
+		res, err := r.Resolve(&model.Media{
+			ID:            1,
+			ThumbnailPath: missing,
+			AbsPath:       imgPath,
+			Type:          model.MediaTypeImage,
+		})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if res.Path != imgPath {
+			t.Fatalf("path = %q, want fallback %q", res.Path, imgPath)
+		}
+	})
+
+	t.Run("non-image with missing thumb returns wrapped stat error", func(t *testing.T) {
+		missing := filepath.Join(tmpDir, "nope.jpg")
+		_, err := r.Resolve(&model.Media{
+			ID:            1,
+			ThumbnailPath: missing,
+			AbsPath:       imgPath,
+			Type:          model.MediaTypeVideo,
+		})
+		if err == nil {
+			t.Fatal("expected error for missing video thumb")
+		}
+		if errors.Is(err, ErrNotFound) {
+			t.Fatalf("did not expect ErrNotFound for video, got %v", err)
+		}
+	})
+
+	t.Run("image with missing thumb and missing AbsPath surfaces stat error", func(t *testing.T) {
+		_, err := r.Resolve(&model.Media{
+			ID:            1,
+			ThumbnailPath: filepath.Join(tmpDir, "gone.jpg"),
+			AbsPath:       filepath.Join(tmpDir, "also-gone.jpg"),
+			Type:          model.MediaTypeImage,
+		})
+		if err == nil {
+			t.Fatal("expected error when both files are missing")
+		}
+	})
+}