Merge branch 'master' into fix/artwork-no-absent-retry

cmd/root.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"context"
+	"net/http"
 	"os"
 	"os/signal"
 	"strings"
@@ -138,7 +139,7 @@ func startServer(ctx context.Context) func() error {
 			a.MountRouter("Prometheus metrics", conf.Server.Prometheus.MetricsPath, p.GetHandler())
 		}
 		if conf.Server.DevEnableProfiler {
-			a.MountRouter("Profiling", "/debug", middleware.Profiler())
+			a.MountRouter("Profiling", "/debug", profilerHandler())
 		}
 		if strings.HasPrefix(conf.Server.UILoginBackgroundURL, "/") {
 			a.MountRouter("Background images", conf.Server.UILoginBackgroundURL, backgrounds.NewHandler())
@@ -147,6 +148,14 @@ func startServer(ctx context.Context) func() error {
 	}
 }
 
+// profilerHandler returns the pprof handler. net/http/pprof resolves the profile
+// name from the raw request path, so the BasePath has to come off first.
+func profilerHandler() http.Handler {
+	// A trailing or root slash would make StripPrefix drop the leading slash chi needs.
+	basePath := strings.TrimRight(conf.Server.BasePath, "/")
+	return http.StripPrefix(basePath, middleware.Profiler())
+}
+
 // schedulePeriodicScan schedules a periodic scan of the music library, if configured.
 func schedulePeriodicScan(ctx context.Context) func() error {
 	return func() error {

cmd/root_test.go

@@ -0,0 +1,46 @@
+package cmd
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"path"
+	"runtime/pprof"
+
+	"github.com/go-chi/chi/v5"
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/conf/configtest"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = pprof.NewProfile("nd-profiler-test")
+
+var _ = Describe("profilerHandler", func() {
+	// Mirrors how server.MountRouter mounts the handler.
+	mount := func() http.Handler {
+		router := chi.NewRouter()
+		router.Mount(path.Join(conf.Server.BasePath, "/debug"), profilerHandler())
+		return router
+	}
+
+	BeforeEach(func() {
+		DeferCleanup(configtest.SetupConfig())
+	})
+
+	DescribeTable("serves a named profile",
+		func(basePath string) {
+			conf.Server.BasePath = basePath
+
+			w := httptest.NewRecorder()
+			target := path.Join(basePath, "/debug/pprof/nd-profiler-test") + "?debug=1"
+			mount().ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil))
+
+			Expect(w.Code).To(Equal(http.StatusOK))
+			Expect(w.Body.String()).To(HavePrefix("nd-profiler-test profile: total 0"))
+		},
+		Entry("without a BasePath", ""),
+		Entry("with a BasePath", "/music"),
+		Entry("with a root BasePath", "/"),
+		Entry("with a trailing-slash BasePath", "/music/"),
+	)
+})

core/artwork/sources.go

@@ -10,9 +10,7 @@ import (
 	"net/http"
 	"net/url"
 	"path/filepath"
-	"reflect"
 	"regexp"
-	"runtime"
 	"strings"
 	"time"
 
@@ -29,16 +27,6 @@ var errSourceUnreadable = errors.New("artwork source unreadable")
 
 type sourceFunc func() (r io.ReadCloser, path string, err error)
 
-func (f sourceFunc) String() string {
-	name := runtime.FuncForPC(reflect.ValueOf(f).Pointer()).Name()
-	name = strings.TrimPrefix(name, "github.com/navidrome/navidrome/core/artwork.")
-	if _, after, found := strings.Cut(name, ")."); found {
-		name = after
-	}
-	name = strings.TrimSuffix(name, ".func1")
-	return name
-}
-
 func fromExternalFile(ctx context.Context, libFS fs.FS, files []string, pattern string) sourceFunc {
 	return func() (io.ReadCloser, string, error) {
 		var openErr error

db/db.go

@@ -6,6 +6,7 @@ import (
 	"embed"
 	"errors"
 	"fmt"
+	"sync"
 	"time"
 
 	"github.com/mattn/go-sqlite3"
@@ -33,15 +34,21 @@ var embedMigrations embed.FS
 
 const migrationsFolder = "migrations"
 
+// sql.Register panics if called twice, so guard it: the singleton instance can be reset
+// (tests/benchmarks) and rebuilt, but the driver is process-global and registers only once.
+var registerDriverOnce sync.Once
+
 func Db() *sql.DB {
 	return singleton.GetInstance(func() *sql.DB {
-		sql.Register(Driver, &sqlite3.SQLiteDriver{
-			ConnectHook: func(conn *sqlite3.SQLiteConn) error {
-				if err := conn.RegisterFunc("SEEDEDRAND", hasher.HashFunc(), false); err != nil {
-					return err
-				}
-				return conn.RegisterCollation(NaturalCollation, natural.CompareFold)
-			},
+		registerDriverOnce.Do(func() {
+			sql.Register(Driver, &sqlite3.SQLiteDriver{
+				ConnectHook: func(conn *sqlite3.SQLiteConn) error {
+					if err := conn.RegisterFunc("SEEDEDRAND", hasher.HashFunc(), false); err != nil {
+						return err
+					}
+					return conn.RegisterCollation(NaturalCollation, natural.CompareFold)
+				},
+			})
 		})
 		Path = conf.Server.DbPath
 		if Path == ":memory:" {

model/metadata/metadata.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 	"strings"
 	"time"
+	"unicode/utf8"
 
 	"github.com/google/uuid"
 	"github.com/navidrome/navidrome/consts"
@@ -366,6 +367,14 @@ func sanitize(filePath string, tagName model.TagName, tag model.TagConf, value s
 	if len(value) > maxLength {
 		log.Trace("Truncated tag value", "tag", tagName, "value", value, "length", len(value), "maxLength", maxLength)
 		value = value[:maxLength]
+		// Drop the partial rune the cut may have left: at most 3 trailing bytes,
+		// so a pre-existing invalid run elsewhere is never consumed.
+		for range 3 {
+			if r, size := utf8.DecodeLastRuneInString(value); r != utf8.RuneError || size != 1 {
+				break
+			}
+			value = value[:len(value)-1]
+		}
 	}
 
 	switch tag.Type {
@@ -387,11 +396,14 @@ func sanitize(filePath string, tagName model.TagName, tag model.TagConf, value s
 			return ""
 		}
 	case model.TagTypeUUID:
-		_, err := uuid.Parse(value)
+		u, err := uuid.Parse(value)
 		if err != nil {
 			log.Trace("Invalid UUID tag value", "tag", tagName, "value", value)
 			return ""
 		}
+		// Store the canonical form: uuid.Parse accepts braces, urn: prefixes and any
+		// two-byte wrapper, and a wrapped value would never match an exact-match query
+		value = u.String()
 	}
 	return value
 }

model/metadata/metadata_internal_test.go

@@ -0,0 +1,20 @@
+package metadata
+
+import (
+	"github.com/navidrome/navidrome/model"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = DescribeTable("sanitize truncation with a tiny MaxLength",
+	// A value of only continuation bytes drains the partial-rune loop to empty; the loop
+	// must stop there instead of slicing value[:-1] and panicking.
+	func(maxLength int, value string) {
+		Expect(func() {
+			Expect(sanitize("file.mp3", "title", model.TagConf{MaxLength: maxLength}, value)).To(Equal(""))
+		}).NotTo(Panic())
+	},
+	Entry("maxLength 1", 1, "\x80\x80"),
+	Entry("maxLength 2", 2, "\x80\x80\x80"),
+	Entry("maxLength 3", 3, "\x80\x80\x80\x80"),
+)

model/metadata/metadata_test.go

@@ -4,6 +4,7 @@ import (
 	"os"
 	"strings"
 	"time"
+	"unicode/utf8"
 
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/metadata"
@@ -122,6 +123,43 @@ var _ = Describe("Metadata", func() {
 				Expect(pair[0].Value()).To(HaveLen(1048570))
 			})
 
+			It("should not split a multi-byte character when truncating", func() {
+				// 1024 is not a multiple of 3, so a byte-wise cut lands mid-rune.
+				props.Tags = model.RawTags{
+					"Title": {strings.Repeat("日", 2048)},
+				}
+				md = metadata.New(filePath, props)
+
+				title := md.String(model.TagTitle)
+				Expect(utf8.ValidString(title)).To(BeTrue(), "truncation produced invalid UTF-8")
+				Expect(len(title)).To(BeNumerically("<=", 1024))
+			})
+
+			It("should keep invalid bytes that are not at the truncation point", func() {
+				props.Tags = model.RawTags{
+					"Title": {"a\xffb" + strings.Repeat("c", 2048)},
+				}
+				md = metadata.New(filePath, props)
+
+				Expect(md.String(model.TagTitle)).To(HaveLen(1024))
+			})
+
+			DescribeTable("should normalize UUID tags to their canonical form",
+				func(raw, expected string) {
+					props.Tags = model.RawTags{"musicbrainz_artistid": {raw}}
+					md = metadata.New(filePath, props)
+
+					Expect(md.String(model.TagMusicBrainzArtistID)).To(Equal(expected))
+				},
+				Entry("canonical", "f81d4fae-7dec-11d0-a765-00a0c91e6bf6", "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"),
+				Entry("uppercase", "F81D4FAE-7DEC-11D0-A765-00A0C91E6BF6", "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"),
+				Entry("braced", "{f81d4fae-7dec-11d0-a765-00a0c91e6bf6}", "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"),
+				Entry("urn prefix", "urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6", "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"),
+				Entry("quoted", `"f81d4fae-7dec-11d0-a765-00a0c91e6bf6"`, "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"),
+				Entry("no dashes", "f81d4fae7dec11d0a76500a0c91e6bf6", "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"),
+				Entry("not a uuid", "the beatles", ""),
+			)
+
 			It("should split multiple values", func() {
 				props.Tags = model.RawTags{
 					"Genre": {"Rock/Pop;;Punk"},

plugins/host_cache.go

@@ -2,6 +2,7 @@ package plugins
 
 import (
 	"context"
+	"runtime"
 	"time"
 
 	"github.com/jellydator/ttlcache/v3"
@@ -29,11 +30,18 @@ func newCacheService(pluginName string) *cacheServiceImpl {
 	// Start the janitor goroutine to clean up expired entries
 	go cache.Start()
 
-	return &cacheServiceImpl{
+	svc := &cacheServiceImpl{
 		pluginName: pluginName,
 		cache:      cache,
 		defaultTTL: defaultCacheTTL,
 	}
+
+	// Automatic cleanup to prevent goroutine leak when the service is garbage collected
+	runtime.AddCleanup(svc, func(ttlCache *ttlcache.Cache[string, any]) {
+		ttlCache.Stop()
+	}, cache)
+
+	return svc
 }
 
 // getTTL converts seconds to a duration, using default if 0 or negative

plugins/host_cache_test.go

@@ -8,9 +8,11 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
+	"runtime"
 	"time"
 
 	"github.com/navidrome/navidrome/conf"
@@ -599,3 +601,35 @@ var _ = Describe("CacheService Integration", Ordered, func() {
 		})
 	})
 })
+
+var _ = Describe("newCacheService", func() {
+	// The suite above leaves goroutines winding down, so settle before sampling.
+	settledBaseline := func() int {
+		var n int
+		Eventually(func() int {
+			runtime.GC()
+			prev := n
+			n = runtime.NumGoroutine()
+			return n - prev
+		}).WithTimeout(10 * time.Second).WithPolling(20 * time.Millisecond).Should(BeZero())
+		return n
+	}
+
+	It("stops the janitor goroutine once the service is unreachable", func() {
+		const numServices = 5
+		baseline := settledBaseline()
+
+		func() {
+			services := make([]*cacheServiceImpl, 0, numServices)
+			for i := range numServices {
+				services = append(services, newCacheService(fmt.Sprintf("plugin_%d", i)))
+			}
+			Expect(runtime.NumGoroutine()).To(BeNumerically(">=", baseline+numServices),
+				"expected one janitor goroutine per cache service")
+		}()
+
+		Eventually(func() int { runtime.GC(); return runtime.NumGoroutine() }).
+			WithTimeout(10*time.Second).WithPolling(20*time.Millisecond).
+			Should(BeNumerically("<=", baseline), "janitor goroutines leaked")
+	})
+})

plugins/host_websocket.go

@@ -243,11 +243,7 @@ func (s *webSocketServiceImpl) getConnection(connectionID string) (*wsConnection
 }
 
 func (s *webSocketServiceImpl) isHostAllowed(host string) bool {
-	// Strip port from host if present
-	hostWithoutPort := host
-	if idx := strings.LastIndex(host, ":"); idx != -1 {
-		hostWithoutPort = host[:idx]
-	}
+	hostWithoutPort := extractHostname(host)
 
 	for _, pattern := range s.requiredHosts {
 		if matchHostPattern(pattern, hostWithoutPort) {

plugins/host_websocket_test.go

@@ -144,20 +144,18 @@ var _ = Describe("WebSocketService", Ordered, func() {
 			Expect(allowed).To(BeFalse())
 		})
 
-		It("should strip port before checking host", func() {
-			// Implementation strips port before matching against patterns
-			// test-websocket manifest has "localhost:*" which matches "localhost"
-			// after port stripping
-			// Note: The port wildcard pattern isn't actually implemented, but
-			// since port is stripped, "localhost:*" is compared against "localhost"
-			// which won't match. To make localhost work, we'd need exact "localhost"
-			// in the allowed hosts list.
-
-			// Testing that port is properly stripped
-			// The pattern "localhost:*" won't match "localhost" due to exact match
-			allowed := testService.isHostAllowed("localhost:8080")
-			Expect(allowed).To(BeFalse())
-		})
+		DescribeTable("should match against the host with its port stripped",
+			func(allowed []string, host string, expected bool) {
+				svc := &webSocketServiceImpl{requiredHosts: allowed}
+				Expect(svc.isHostAllowed(host)).To(Equal(expected))
+			},
+			Entry("hostname with port", []string{"example.com"}, "example.com:8080", true),
+			Entry("IPv6 with port", []string{"::1"}, "[::1]:8080", true),
+			Entry("IPv6 without port", []string{"::1"}, "[::1]", true),
+			Entry("host not in the list", []string{"::2"}, "[::1]:8080", false),
+			// "localhost:*" is matched against the stripped "localhost", so it never hits
+			Entry("port wildcards are not supported", []string{"localhost:*"}, "localhost:8080", false),
+		)
 	})
 
 	Describe("Connection Management", func() {

scanner/scanner_benchmark_test.go

@@ -2,8 +2,8 @@ package scanner_test
 
 import (
 	"context"
+	"database/sql"
 	"fmt"
-	"os"
 	"path/filepath"
 	"runtime"
 	"testing"
@@ -21,6 +21,8 @@ import (
 	"github.com/navidrome/navidrome/persistence"
 	"github.com/navidrome/navidrome/scanner"
 	"github.com/navidrome/navidrome/server/events"
+	"github.com/navidrome/navidrome/tests"
+	"github.com/navidrome/navidrome/utils/singleton"
 	"go.uber.org/goleak"
 )
 
@@ -31,11 +33,25 @@ func BenchmarkScan(b *testing.B) {
 		goleak.IgnoreAnyFunction("testing.(*B).doBench"),
 		// Ignore database/sql.(*DB).connectionOpener, as we are not closing the database connection
 		goleak.IgnoreAnyFunction("database/sql.(*DB).connectionOpener"),
+		// A preceding TestScanner leaves Ginkgo's interrupt handler running.
+		goleak.IgnoreTopFunction("github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2"),
+		// The notify library keeps watcher goroutines alive after Stop(); recursive on macOS, nonrecursive on Linux.
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*recursiveTree).dispatch"),
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*nonrecursiveTree).dispatch"),
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*nonrecursiveTree).internal"),
 	)
 
-	tmpDir := os.TempDir()
+	tests.Init(b, false)
+
+	tmpDir := b.TempDir()
 	conf.Server.DbPath = filepath.Join(tmpDir, "test-scanner.db?_journal_mode=WAL")
-	db.Init(context.Background())
+	// The default library is seeded from MusicFolder, and its path cannot be changed afterwards
+	conf.Server.MusicFolder = "fake:///music"
+	// TestScanner may run first and close the shared DB singleton; drop it so db.Init
+	// opens a fresh one whether or not the test suite ran before this benchmark.
+	singleton.DeleteInstance[*sql.DB]()
+	// Close before b.TempDir cleanup runs, or Windows cannot delete the open DB/WAL files.
+	defer db.Init(context.Background())()
 
 	ds := persistence.New(db.Db())
 	conf.Server.DevExternalScanner = false

scanner/scanner_suite_test.go

@@ -3,7 +3,6 @@ package scanner_test
 import (
 	"context"
 	"io/fs"
-	"os"
 	"testing"
 
 	"github.com/navidrome/navidrome/consts"
@@ -31,16 +30,15 @@ func init() {
 }
 
 func TestScanner(t *testing.T) {
-	// Only run goleak checks when the GOLEAK env var is set
-	if os.Getenv("GOLEAK") != "" {
-		// Detect any goroutine leaks in the scanner code under test
-		defer goleak.VerifyNone(t,
-			goleak.IgnoreTopFunction("github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2"),
-			// The notify library creates internal goroutines for file watching that persist after Stop() is called.
-			// These are created by the plugins package tests and are expected behavior.
-			goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*recursiveTree).dispatch"),
-		)
-	}
+	// Detect any goroutine leaks in the scanner code under test
+	defer goleak.VerifyNone(t,
+		goleak.IgnoreTopFunction("github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2"),
+		// The notify library keeps internal goroutines alive after Stop(). The backend picks the tree per
+		// platform: recursive on macOS (FSEvents), nonrecursive on Linux (inotify), so ignore both.
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*recursiveTree).dispatch"),
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*nonrecursiveTree).dispatch"),
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*nonrecursiveTree).internal"),
+	)
 
 	tests.Init(t, true)
 	defer db.Close(context.Background())

server/backgrounds/backgrounds_suite_test.go

@@ -0,0 +1,17 @@
+package backgrounds
+
+import (
+	"testing"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestBackgrounds(t *testing.T) {
+	tests.Init(t, false)
+	log.SetLevel(log.LevelFatal)
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Backgrounds Suite")
+}

server/backgrounds/handler.go

@@ -81,7 +81,7 @@ func (h *Handler) serveImage(ctx context.Context, item cache.Item) (io.Reader, e
 	}
 	c := httpclient.New(imageRequestTimeout)
 	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, imageURL(image), nil)
-	resp, err := c.Do(req) //nolint:bodyclose,gosec // No need to close resp.Body, it will be closed via the CachedStream wrapper
+	resp, err := c.Do(req) //nolint:bodyclose,gosec // On success the body is closed via the CachedStream wrapper
 	if errors.Is(err, context.DeadlineExceeded) {
 		defaultImage, _ := base64.StdEncoding.DecodeString(consts.DefaultUILoginBackgroundOffline)
 		return strings.NewReader(string(defaultImage)), nil
@@ -90,6 +90,7 @@ func (h *Handler) serveImage(ctx context.Context, item cache.Item) (io.Reader, e
 		return nil, fmt.Errorf("could not get background image from hosting service: %w", err)
 	}
 	if resp.StatusCode != http.StatusOK {
+		_ = resp.Body.Close()
 		return nil, fmt.Errorf("unexpected status code getting background image from hosting service: %d", resp.StatusCode)
 	}
 	log.Debug(ctx, "Got background image from hosting service", "image", image, "elapsed", time.Since(start))

server/backgrounds/handler_test.go

@@ -0,0 +1,68 @@
+package backgrounds
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"strings"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+type recordingBody struct {
+	io.Reader
+	closed *bool
+}
+
+func (b recordingBody) Close() error {
+	*b.closed = true
+	return nil
+}
+
+type stubTransport struct {
+	statusCode int
+	closed     *bool
+}
+
+func (t stubTransport) RoundTrip(*http.Request) (*http.Response, error) {
+	return &http.Response{
+		StatusCode: t.statusCode,
+		Header:     make(http.Header),
+		Body:       recordingBody{Reader: strings.NewReader("image-bytes"), closed: t.closed},
+	}, nil
+}
+
+var _ = Describe("serveImage", func() {
+	var closed bool
+
+	BeforeEach(func() {
+		closed = false
+	})
+
+	stubStatus := func(statusCode int) {
+		original := http.DefaultTransport
+		http.DefaultTransport = stubTransport{statusCode: statusCode, closed: &closed}
+		DeferCleanup(func() { http.DefaultTransport = original })
+	}
+
+	It("closes the response body when the hosting service returns an error", func() {
+		stubStatus(http.StatusNotFound)
+
+		_, err := (&Handler{}).serveImage(context.Background(), cacheKey("some-image.webp"))
+
+		Expect(err).To(MatchError(ContainSubstring("unexpected status code")))
+		Expect(closed).To(BeTrue(), "response body was left open")
+	})
+
+	It("hands the still-open body to the caller on success", func() {
+		stubStatus(http.StatusOK)
+
+		reader, err := (&Handler{}).serveImage(context.Background(), cacheKey("some-image.webp"))
+
+		Expect(err).ToNot(HaveOccurred())
+		Expect(closed).To(BeFalse(), "response body must stay open for the CachedStream wrapper")
+		body, _ := io.ReadAll(reader)
+		Expect(string(body)).To(Equal("image-bytes"))
+	})
+})

tests/init_tests.go

@@ -13,7 +13,7 @@ import (
 
 var once sync.Once
 
-func Init(t *testing.T, skipOnShort bool) {
+func Init(t testing.TB, skipOnShort bool) {
 	if skipOnShort && testing.Short() {
 		t.Skip("skipping test in short mode.")
 	}

utils/singleton/singleton.go

@@ -67,3 +67,13 @@ func GetInstance[T any](constructor func() T) T {
 
 	return newInstance
 }
+
+// DeleteInstance drops the cached instance of type T so the next GetInstance rebuilds it.
+// Intended for tests and benchmarks that need a fresh instance regardless of run order.
+func DeleteInstance[T any]() {
+	var v T
+	name := reflect.TypeOf(v).String()
+	lock.Lock()
+	delete(instances, name)
+	lock.Unlock()
+}