fix: address Codex review on the pprof base path and scan benchmark

- profilerHandler: treat a root BasePath ("/") as no prefix, so http.StripPrefix
  keeps the leading slash chi needs; without this the profiler 404s when BaseURL
  is "/". Cover the root case in the test.
- BenchmarkScan: make it run regardless of test/benchmark ordering. Add
  singleton.DeleteInstance so a fresh DB is opened after TestScanner closes the
  shared one, guard driver registration with sync.Once so the rebuild does not
  re-Register, and ignore the Ginkgo interrupt-handler and Linux notify
  goroutines the preceding suite leaves behind.
This commit is contained in:
Deluan 2026-08-30 17:25:28 -04:00
parent 6045f78709
commit b5089d380c
5 changed files with 41 additions and 11 deletions

View File

@ -151,7 +151,11 @@ 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 {
return http.StripPrefix(conf.Server.BasePath, middleware.Profiler())
basePath := conf.Server.BasePath
if basePath == "/" { // StripPrefix("/") would drop the leading slash chi needs
basePath = ""
}
return http.StripPrefix(basePath, middleware.Profiler())
}
// schedulePeriodicScan schedules a periodic scan of the music library, if configured.

View File

@ -32,7 +32,7 @@ var _ = Describe("profilerHandler", func() {
conf.Server.BasePath = basePath
w := httptest.NewRecorder()
target := basePath + "/debug/pprof/nd-profiler-test?debug=1"
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))
@ -40,5 +40,6 @@ var _ = Describe("profilerHandler", func() {
},
Entry("without a BasePath", ""),
Entry("with a BasePath", "/music"),
Entry("with a root BasePath", "/"),
)
})

View File

@ -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:" {

View File

@ -2,6 +2,7 @@ package scanner_test
import (
"context"
"database/sql"
"fmt"
"path/filepath"
"runtime"
@ -21,6 +22,7 @@ import (
"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,17 +33,23 @@ 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"),
// The notify library keeps internal watcher goroutines alive after Stop()
// 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"),
)
// go test -bench runs with -run=XXX, so TestScanner never loads the test config
tests.Init(b, false)
tmpDir := b.TempDir()
conf.Server.DbPath = filepath.Join(tmpDir, "test-scanner.db?_journal_mode=WAL")
// 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]()
db.Init(context.Background())
ds := persistence.New(db.Db())

View File

@ -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()
}