From 116a4407184b6f21221a6aa7702fb738a3915773 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 10 Jul 2026 18:43:31 -0400 Subject: [PATCH 1/6] test(scanner): fix flaky Windows search_normalized rescan test (#5758) The 'repopulates a stale search_normalized on a full rescan' spec runs two full scans back-to-back. Whether the second scan refreshes the unchanged artist depends on folderEntry.isOutdated(), which compares folder.updated_at (written during the first scan) against the second scan's library.last_scan_started_at using a strict time.Before(). Both are time.Now() values captured milliseconds apart. On Linux's fine-grained clock they are always distinct, so the test passes. On Windows the coarse wall-clock granularity frequently makes the two timestamps land in the same tick and compare equal, so Before() returns false, the folder is treated as up-to-date and skipped, the artist is never re-persisted, and search_normalized stays empty -- failing the assertion intermittently across unrelated PRs. Backdate the folder's updated_at an hour before the second scan so the comparison is unambiguous on every platform. This is a test-only timing artifact (real rescans never run milliseconds apart on an unchanged library), so no production code changes are needed. --- scanner/scanner_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 7f3dca775..cc3720717 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -6,6 +6,7 @@ import ( "errors" "path/filepath" "testing/fstest" + "time" "github.com/Masterminds/squirrel" "github.com/google/uuid" @@ -212,6 +213,15 @@ var _ = Describe("Scanner", Ordered, func() { _, err := db.Db().ExecContext(ctx, "UPDATE artist SET search_normalized = '' WHERE name = 'GØGGS'") Expect(err).ToNot(HaveOccurred()) + // Backdate the folder so the next full scan reliably sees it as outdated. + // isOutdated() compares folder.updated_at (written by this scan) against the + // next scan's last_scan_started_at with a strict Before(); back-to-back scans + // can capture both within one clock tick on Windows (coarse wall-clock), making + // the refresh flaky. Backdating forces the comparison to be unambiguous. + _, err = db.Db().ExecContext(ctx, + "UPDATE folder SET updated_at = ?", time.Now().Add(-time.Hour)) + Expect(err).ToNot(HaveOccurred()) + Expect(runScanner(ctx, true)).To(Succeed()) Expect(searchNormalized()).To(Equal("GOGGS")) }) From 205c85da5523338807808202d8877ccaa6e82776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 10 Jul 2026 19:15:49 -0400 Subject: [PATCH 2/6] fix(plugins): surface host service failures when loading plugins (#5756) * fix(plugins): surface host service failures when loading plugins When a host service failed to initialize during plugin load (e.g. the taskqueue database could not be created because the data folder is not writable), the error was logged and swallowed, and its host functions were silently omitted. Instantiation then failed with a misleading error such as '"task_createqueue" is not exported in module "extism:host/user"', which reads as a plugin/host API mismatch and gets wrongly blamed on plugin authors (see kgarner7/navidrome-listenbrainz-daily-playlist#26). Host service factories now return an error, and loadPluginWithConfig fails fast with the actual cause (e.g. 'creating Task service: creating plugin data directory: ...'), which is also stored in the plugin's last_error. Closers accumulated before a load failure are now closed (via a deferred guard covering all failure paths), so partially-created services no longer leak goroutines or database handles. Also fix a panic in 'navidrome plugin enable': CLI commands use the plugin Manager without calling Start, so manager.ctx was nil and newTaskQueueService panicked in context.WithCancel. Long-lived host services (taskqueue, kvstore, websocket) now receive their lifecycle context explicitly in the constructor, sourced from serviceContext.baseCtx(), which falls back to context.Background() for the unstarted-manager case. * docs(plugins): correct websocket readLoop lifecycle comment The comment claimed the read loop's context is always cancelled during application shutdown, which is not true when the manager was never started (one-shot CLI runs, where baseCtx falls back to context.Background()). Clarify that connection closure via Close() on plugin unload is what ends the read loop, with context cancellation as a server-shutdown backstop. Addresses review feedback on #5756. * test(plugins): exclude plugin-loading specs from Windows builds The new loadPluginWithConfig specs reference test suite helpers (testdataDir, noopMetricsRecorder) defined in plugins_suite_test.go, which is excluded on Windows, breaking test compilation there. Move the specs to their own file with the same build constraint, keeping the pure-function specs in manager_loader_test.go running on Windows as before. --- plugins/host_taskqueue.go | 5 +- plugins/host_taskqueue_test.go | 16 +---- plugins/host_websocket.go | 15 +++-- plugins/manager_loader.go | 98 +++++++++++++++++++---------- plugins/manager_loader_load_test.go | 78 +++++++++++++++++++++++ 5 files changed, 158 insertions(+), 54 deletions(-) create mode 100644 plugins/manager_loader_load_test.go diff --git a/plugins/host_taskqueue.go b/plugins/host_taskqueue.go index a5db3344f..2f74c0aa4 100644 --- a/plugins/host_taskqueue.go +++ b/plugins/host_taskqueue.go @@ -82,7 +82,8 @@ type taskQueueServiceImpl struct { } // newTaskQueueService creates a new taskQueueServiceImpl with its own SQLite database. -func newTaskQueueService(pluginName string, manager *Manager, maxConcurrency int32) (*taskQueueServiceImpl, error) { +// The given ctx bounds the service's background work (queue workers, cleanup loop). +func newTaskQueueService(ctx context.Context, pluginName string, manager *Manager, maxConcurrency int32) (*taskQueueServiceImpl, error) { dataDir := filepath.Join(conf.Server.DataFolder.String(), "plugins", pluginName) if err := os.MkdirAll(dataDir, 0700); err != nil { return nil, fmt.Errorf("creating plugin data directory: %w", err) @@ -102,7 +103,7 @@ func newTaskQueueService(pluginName string, manager *Manager, maxConcurrency int return nil, fmt.Errorf("creating taskqueue schema: %w", err) } - ctx, cancel := context.WithCancel(manager.ctx) //nolint:gosec // cancel is stored in struct and called in Close() + ctx, cancel := context.WithCancel(ctx) //nolint:gosec // cancel is stored in struct and called in Close() s := &taskQueueServiceImpl{ pluginName: pluginName, diff --git a/plugins/host_taskqueue_test.go b/plugins/host_taskqueue_test.go index faff79c8e..d459fd69b 100644 --- a/plugins/host_taskqueue_test.go +++ b/plugins/host_taskqueue_test.go @@ -42,15 +42,11 @@ var _ = Describe("TaskQueueService", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.DataFolder = conf.NewDir(tmpDir) - // Create a mock manager with context - managerCtx, cancel := context.WithCancel(ctx) manager = &Manager{ plugins: make(map[string]*plugin), - ctx: managerCtx, } - DeferCleanup(cancel) - service, err = newTaskQueueService("test_plugin", manager, 5) + service, err = newTaskQueueService(ctx, "test_plugin", manager, 5) Expect(err).ToNot(HaveOccurred()) }) @@ -730,14 +726,11 @@ var _ = Describe("TaskQueueService", func() { service.Close() // Create a new service pointing to the same DB - managerCtx2, cancel2 := context.WithCancel(ctx) - DeferCleanup(cancel2) manager2 := &Manager{ plugins: make(map[string]*plugin), - ctx: managerCtx2, } - service, err = newTaskQueueService("test_plugin", manager2, 5) + service, err = newTaskQueueService(ctx, "test_plugin", manager2, 5) Expect(err).ToNot(HaveOccurred()) // Override callback to succeed @@ -775,14 +768,11 @@ var _ = Describe("TaskQueueService", func() { Describe("Plugin isolation", func() { It("uses separate databases for different plugins", func() { - managerCtx2, cancel2 := context.WithCancel(ctx) - DeferCleanup(cancel2) manager2 := &Manager{ plugins: make(map[string]*plugin), - ctx: managerCtx2, } - service2, err := newTaskQueueService("other_plugin", manager2, 5) + service2, err := newTaskQueueService(ctx, "other_plugin", manager2, 5) Expect(err).ToNot(HaveOccurred()) defer service2.Close() diff --git a/plugins/host_websocket.go b/plugins/host_websocket.go index eef1e6236..90403f4c0 100644 --- a/plugins/host_websocket.go +++ b/plugins/host_websocket.go @@ -54,6 +54,7 @@ type wsConnection struct { // webSocketServiceImpl implements host.WebSocketService. // It provides plugins with WebSocket communication capabilities. type webSocketServiceImpl struct { + baseCtx context.Context // bounds the read loops, which outlive the Connect() call pluginName string manager *Manager requiredHosts []string @@ -63,8 +64,9 @@ type webSocketServiceImpl struct { } // newWebSocketService creates a new WebSocketService for a plugin. -func newWebSocketService(pluginName string, manager *Manager, permission *WebSocketPermission) *webSocketServiceImpl { +func newWebSocketService(ctx context.Context, pluginName string, manager *Manager, permission *WebSocketPermission) *webSocketServiceImpl { return &webSocketServiceImpl{ + baseCtx: ctx, pluginName: pluginName, manager: manager, requiredHosts: permission.RequiredHosts, @@ -129,11 +131,12 @@ func (s *webSocketServiceImpl) Connect(ctx context.Context, urlStr string, heade s.connections[connectionID] = wsConn s.mu.Unlock() - // Start read goroutine with manager's context. - // We use manager.ctx instead of the caller's ctx because the readLoop must - // outlive the Connect() call. The manager's context is cancelled during - // application shutdown, ensuring graceful cleanup. - go s.readLoop(s.manager.ctx, connectionID, wsConn) + // Start read goroutine with the service's base context instead of the + // caller's ctx, because the readLoop must outlive the Connect() call. + // Connections are closed by Close() when the plugin is unloaded, which ends + // the readLoop; the base context is a backstop that also ends it on server + // shutdown (it is never cancelled in one-shot CLI runs). + go s.readLoop(s.baseCtx, connectionID, wsConn) log.Debug(ctx, "WebSocket connected", "plugin", s.pluginName, "connectionID", connectionID, "url", urlStr) return connectionID, nil diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index 604fba3a7..757ededb5 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -30,11 +30,23 @@ type serviceContext struct { allLibraries bool // If true, plugin can access all libraries } +// baseCtx returns the manager's lifecycle context, for host services that +// outlive the plugin call that created them. It falls back to +// context.Background() when the manager was never started, which is the case +// for CLI commands (e.g. `navidrome plugin enable`) that load plugins without +// calling Start. +func (c *serviceContext) baseCtx() context.Context { + if c.manager.ctx == nil { + return context.Background() + } + return c.manager.ctx +} + // hostServiceEntry defines a host service for table-driven registration. type hostServiceEntry struct { name string hasPermission func(*Permissions) bool - create func(*serviceContext) ([]extism.HostFunction, io.Closer) + create func(*serviceContext) ([]extism.HostFunction, io.Closer, error) } // hostServices defines all available host services. @@ -43,119 +55,117 @@ var hostServices = []hostServiceEntry{ { name: "Config", hasPermission: func(p *Permissions) bool { return true }, // Always available, no permission required - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newConfigService(ctx.pluginName, ctx.config) - return host.RegisterConfigHostFunctions(service), nil + return host.RegisterConfigHostFunctions(service), nil, nil }, }, { name: "SubsonicAPI", hasPermission: func(p *Permissions) bool { return p != nil && p.Subsonicapi != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newSubsonicAPIService(ctx.pluginName, ctx.manager.subsonicRouter, ctx.manager.ds, newUserAccess(ctx.allowedUsers, ctx.allUsers)) - return host.RegisterSubsonicAPIHostFunctions(service), nil + return host.RegisterSubsonicAPIHostFunctions(service), nil, nil }, }, { name: "Scheduler", hasPermission: func(p *Permissions) bool { return p != nil && p.Scheduler != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newSchedulerService(ctx.pluginName, ctx.manager, scheduler.GetInstance()) - return host.RegisterSchedulerHostFunctions(service), service + return host.RegisterSchedulerHostFunctions(service), service, nil }, }, { name: "WebSocket", hasPermission: func(p *Permissions) bool { return p != nil && p.Websocket != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Websocket - service := newWebSocketService(ctx.pluginName, ctx.manager, perm) - return host.RegisterWebSocketHostFunctions(service), service + service := newWebSocketService(ctx.baseCtx(), ctx.pluginName, ctx.manager, perm) + return host.RegisterWebSocketHostFunctions(service), service, nil }, }, { name: "Artwork", hasPermission: func(p *Permissions) bool { return p != nil && p.Artwork != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newArtworkService() - return host.RegisterArtworkHostFunctions(service), nil + return host.RegisterArtworkHostFunctions(service), nil, nil }, }, { name: "Cache", hasPermission: func(p *Permissions) bool { return p != nil && p.Cache != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newCacheService(ctx.pluginName) - return host.RegisterCacheHostFunctions(service), service + return host.RegisterCacheHostFunctions(service), service, nil }, }, { name: "Library", hasPermission: func(p *Permissions) bool { return p != nil && p.Library != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Library service := newLibraryService(ctx.manager.ds, perm, ctx.allowedLibraries, ctx.allLibraries) - return host.RegisterLibraryHostFunctions(service), nil + return host.RegisterLibraryHostFunctions(service), nil, nil }, }, { name: "KVStore", hasPermission: func(p *Permissions) bool { return p != nil && p.Kvstore != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Kvstore - service, err := newKVStoreService(ctx.manager.ctx, ctx.pluginName, perm) + service, err := newKVStoreService(ctx.baseCtx(), ctx.pluginName, perm) if err != nil { - log.Error("Failed to create KVStore service", "plugin", ctx.pluginName, err) - return nil, nil + return nil, nil, err } - return host.RegisterKVStoreHostFunctions(service), service + return host.RegisterKVStoreHostFunctions(service), service, nil }, }, { name: "Users", hasPermission: func(p *Permissions) bool { return p != nil && p.Users != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newUsersService(ctx.manager.ds, ctx.allowedUsers, ctx.allUsers) - return host.RegisterUsersHostFunctions(service), nil + return host.RegisterUsersHostFunctions(service), nil, nil }, }, { name: "Matcher", hasPermission: func(p *Permissions) bool { return p != nil && p.Matcher != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { hasFilesystemPerm := ctx.permissions.Library != nil && ctx.permissions.Library.Filesystem service := newMatcherService( ctx.manager.ds, hasFilesystemPerm, newUserAccess(ctx.allowedUsers, ctx.allUsers), newLibraryAccess(ctx.allowedLibraries, ctx.allLibraries), ) - return host.RegisterMatcherHostFunctions(service), nil + return host.RegisterMatcherHostFunctions(service), nil, nil }, }, { name: "HTTP", hasPermission: func(p *Permissions) bool { return p != nil && p.Http != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Http service := newHTTPService(ctx.pluginName, perm) - return host.RegisterHTTPHostFunctions(service), nil + return host.RegisterHTTPHostFunctions(service), nil, nil }, }, { name: "Task", hasPermission: func(p *Permissions) bool { return p != nil && p.Taskqueue != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Taskqueue maxConcurrency := int32(1) if perm.MaxConcurrency > 0 { maxConcurrency = int32(perm.MaxConcurrency) } - service, err := newTaskQueueService(ctx.pluginName, ctx.manager, maxConcurrency) + service, err := newTaskQueueService(ctx.baseCtx(), ctx.pluginName, ctx.manager, maxConcurrency) if err != nil { - log.Error("Failed to create Task service", "plugin", ctx.pluginName, err) - return nil, nil + return nil, nil, err } - return host.RegisterTaskHostFunctions(service), service + return host.RegisterTaskHostFunctions(service), service, nil }, }, } @@ -256,6 +266,7 @@ func (m *Manager) loadEnabledPlugins(ctx context.Context) error { // loadPluginWithConfig loads a plugin with configuration from DB. // The p.Path should point to an .ndp package file. func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { + // NewContext falls back to context.Background() when m.ctx is nil (unstarted manager) ctx := log.NewContext(m.ctx, "plugin", p.ID) if m.stopped.Load() { @@ -328,6 +339,15 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { // Build host functions based on permissions from manifest var hostFunctions []extism.HostFunction var closers []io.Closer + loaded := false + // On success the closers are owned by the registered plugin; on any + // failure past this point, close them so partially-created services + // don't leak goroutines or file handles. + defer func() { + if !loaded { + closeAll(closers) + } + }() svcCtx := &serviceContext{ pluginName: p.ID, @@ -341,7 +361,10 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { } for _, entry := range hostServices { if entry.hasPermission(pkg.Manifest.Permissions) { - funcs, closer := entry.create(svcCtx) + funcs, closer, err := entry.create(svcCtx) + if err != nil { + return fmt.Errorf("creating %s service: %w", entry.name, err) + } hostFunctions = append(hostFunctions, funcs...) if closer != nil { closers = append(closers, closer) @@ -400,6 +423,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { libraries: newLibraryAccess(allowedLibraries, p.AllLibraries), } m.mu.Unlock() + loaded = true // Call plugin init function callPluginInit(ctx, m.plugins[p.ID]) @@ -407,6 +431,14 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { return nil } +// closeAll closes host service closers accumulated before a load failure, +// so partially-created services don't leak goroutines or file handles. +func closeAll(closers []io.Closer) { + for _, c := range closers { + _ = c.Close() + } +} + // parsePluginConfig parses a JSON config string into a map of string values. // For Extism, all config values must be strings, so non-string values are serialized as JSON. func parsePluginConfig(configJSON string) (map[string]string, error) { diff --git a/plugins/manager_loader_load_test.go b/plugins/manager_loader_load_test.go new file mode 100644 index 000000000..8f35548af --- /dev/null +++ b/plugins/manager_loader_load_test.go @@ -0,0 +1,78 @@ +//go:build !windows + +package plugins + +import ( + "crypto/sha256" + "encoding/hex" + "net/http" + "os" + "path/filepath" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("loadPluginWithConfig", func() { + var manager *Manager + var dataDir string + + BeforeEach(func() { + pluginsDir := GinkgoT().TempDir() + dataDir = GinkgoT().TempDir() + + src := filepath.Join(testdataDir, "test-taskqueue"+PackageExtension) + data, err := os.ReadFile(src) + Expect(err).ToNot(HaveOccurred()) + dest := filepath.Join(pluginsDir, "test-taskqueue"+PackageExtension) + Expect(os.WriteFile(dest, data, 0600)).To(Succeed()) + hash := sha256.Sum256(data) + + DeferCleanup(configtest.SetupConfig()) + conf.Server.Plugins.Enabled = true + conf.Server.Plugins.Folder = conf.NewDir(pluginsDir) + conf.Server.Plugins.AutoReload = false + conf.Server.DataFolder = conf.NewDir(dataDir) + + repo := tests.CreateMockPluginRepo() + repo.Permitted = true + repo.SetData(model.Plugins{{ + ID: "test-taskqueue", + Path: dest, + SHA256: hex.EncodeToString(hash[:]), + Enabled: false, + }}) + manager = &Manager{ + plugins: make(map[string]*plugin), + ds: &tests.MockDataStore{MockedPlugin: repo}, + metrics: noopMetricsRecorder{}, + subsonicRouter: http.NotFoundHandler(), + } + }) + + Describe("host service creation failures", func() { + It("reports the Task service creation error instead of a missing host function", func() { + Expect(manager.Start(GinkgoT().Context())).To(Succeed()) + DeferCleanup(func() { _ = manager.Stop() }) + + // Block the taskqueue data dir by creating a file where the directory should be + Expect(os.WriteFile(filepath.Join(dataDir, "plugins"), nil, 0600)).To(Succeed()) + + err := manager.EnablePlugin(GinkgoT().Context(), "test-taskqueue") + Expect(err).To(MatchError(ContainSubstring("creating Task service"))) + Expect(err).ToNot(MatchError(ContainSubstring("not exported"))) + }) + }) + + Describe("unstarted manager", func() { + It("enables a taskqueue plugin on a manager that was never started", func() { + // CLI commands (navidrome plugin enable) use the manager without calling Start + Expect(manager.EnablePlugin(GinkgoT().Context(), "test-taskqueue")).To(Succeed()) + DeferCleanup(func() { _ = manager.unloadPlugin("test-taskqueue") }) + }) + }) +}) From e91687e760a8b4e19dc74b6a9b1ff7f3674565ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 10 Jul 2026 20:27:29 -0400 Subject: [PATCH 3/6] fix(smartplaylist): reject NSP mixing top-level 'any' and 'all' (#5759) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(scanner): fix flaky Windows search_normalized rescan test The 'repopulates a stale search_normalized on a full rescan' spec runs two full scans back-to-back. Whether the second scan refreshes the unchanged artist depends on folderEntry.isOutdated(), which compares folder.updated_at (written during the first scan) against the second scan's library.last_scan_started_at using a strict time.Before(). Both are time.Now() values captured milliseconds apart. On Linux's fine-grained clock they are always distinct, so the test passes. On Windows the coarse wall-clock granularity frequently makes the two timestamps land in the same tick and compare equal, so Before() returns false, the folder is treated as up-to-date and skipped, the artist is never re-persisted, and search_normalized stays empty -- failing the assertion intermittently across unrelated PRs. Backdate the folder's updated_at an hour before the second scan so the comparison is unambiguous on every platform. This is a test-only timing artifact (real rescans never run milliseconds apart on an unchanged library), so no production code changes are needed. * fix(smartplaylist): reject NSP mixing top-level 'any' and 'all' A smart playlist (.nsp) that specified both a top-level "any" and a top-level "all" group was imported by silently keeping only "any" and discarding "all", regardless of key order. The Criteria model holds a single top-level Expression, so it cannot represent both groups, and the parser picked "any" without reporting the dropped rules. Make Criteria.UnmarshalJSON return an error when both keys are present at the top level, so the scanner fails loudly (logging the playlist as invalid) instead of silently losing rules. Users should nest one group inside the other, as shown in the documented examples. Fixes #5757 * fix(smartplaylist): reject top-level any+all by key presence Address code review feedback: the previous guard checked decoded slice lengths, so it only rejected the mixed top-level any/all form when both groups were non-empty. An input like {"any":[],"all":[...]} (or a null group) slipped past and silently used just one group — the same class of silent drop this change set out to prevent. Decode the two keys as json.RawMessage and detect presence by key rather than length, so any file that provides both top-level keys is rejected regardless of whether one group is empty or null. * refactor(smartplaylist): detect top-level any+all via presence type Replace the json.RawMessage + manual double-unmarshal in Criteria.UnmarshalJSON with a small optionalConjunction wrapper whose UnmarshalJSON records that its key was present. Because encoding/json invokes UnmarshalJSON even for a JSON null, this keeps the exact behavior (a present-but-empty or null group still counts, so mixing both top-level keys is rejected) while decoding in a single pass — no raw-message capture, no re-decode, no shadow variables. No behavior change; existing tests pass unchanged. --- core/playlists/parse_nsp_test.go | 14 ++++++++++++++ model/criteria/criteria.go | 27 ++++++++++++++++----------- model/criteria/criteria_test.go | 22 ++++++++++++++++++++++ model/criteria/json.go | 14 ++++++++++++++ 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/core/playlists/parse_nsp_test.go b/core/playlists/parse_nsp_test.go index 516a5355d..d6d69866f 100644 --- a/core/playlists/parse_nsp_test.go +++ b/core/playlists/parse_nsp_test.go @@ -113,6 +113,20 @@ var _ = Describe("parseNSP", func() { Expect(err.Error()).To(ContainSubstring("SmartPlaylist")) }) + It("rejects a NSP that mixes top-level 'any' and 'all' instead of silently dropping a group", func() { + nsp := `{ + "name": "Overplayed Favorites", + "any": [{"inPlaylist": {"path": "most-played-favorites.nsp"}}], + "all": [{"notInPlaylist": {"path": "favorites-not-played-in-4-yrs.nsp"}}], + "sort": "playCount, lastPlayed" + }` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("SmartPlaylist")) + Expect(err.Error()).To(And(ContainSubstring("all"), ContainSubstring("any"))) + }) + It("gracefully handles non-string name field", func() { nsp := `{"name": 123, "all": [{"is": {"loved": true}}]}` pls := &model.Playlist{Name: "Original"} diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index 31d208d08..8c3d183a9 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -103,21 +103,26 @@ func (c Criteria) MarshalJSON() ([]byte, error) { func (c *Criteria) UnmarshalJSON(data []byte) error { var aux struct { - All unmarshalConjunctionType `json:"all"` - Any unmarshalConjunctionType `json:"any"` - Sort string `json:"sort"` - Order string `json:"order"` - Limit int `json:"limit"` - LimitPercent int `json:"limitPercent"` - Offset int `json:"offset"` + All optionalConjunction `json:"all"` + Any optionalConjunction `json:"any"` + Sort string `json:"sort"` + Order string `json:"order"` + Limit int `json:"limit"` + LimitPercent int `json:"limitPercent"` + Offset int `json:"offset"` } if err := json.Unmarshal(data, &aux); err != nil { return err } - if len(aux.Any) > 0 { - c.Expression = Any(aux.Any) - } else if len(aux.All) > 0 { - c.Expression = All(aux.All) + // A Criteria has a single top-level group. Reject files that provide both keys + // (even when one is [] or null) rather than silently dropping one of them. + if aux.All.present && aux.Any.present { + return errors.New("invalid criteria json: 'all' and 'any' cannot both be used at the top level; nest one inside the other instead") + } + if len(aux.Any.rules) > 0 { + c.Expression = Any(aux.Any.rules) + } else if len(aux.All.rules) > 0 { + c.Expression = All(aux.All.rules) } else { return errors.New("invalid criteria json. missing rules (key 'all' or 'any')") } diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index 092cfd36a..7f214e703 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -80,6 +80,28 @@ var _ = Describe("Criteria", func() { }) }) + Context("with both top-level 'all' and 'any'", func() { + It("returns an error instead of silently dropping one of the groups", func() { + jsonStr := `{"any":[{"inPlaylist":{"path":"a.nsp"}}],"all":[{"notInPlaylist":{"path":"b.nsp"}}]}` + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(err.Error()).To(gomega.And(gomega.ContainSubstring("all"), gomega.ContainSubstring("any"))) + }) + + DescribeTable("rejects both keys even when one group is present but empty", + func(jsonStr string) { + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(err.Error()).To(gomega.And(gomega.ContainSubstring("all"), gomega.ContainSubstring("any"))) + }, + Entry("empty any", `{"any":[],"all":[{"is":{"loved":true}}]}`), + Entry("empty all", `{"all":[],"any":[{"is":{"loved":true}}]}`), + Entry("null any", `{"any":null,"all":[{"is":{"loved":true}}]}`), + ) + }) + Describe("LimitPercent", func() { Describe("JSON round-trip", func() { It("marshals and unmarshals limitPercent", func() { diff --git a/model/criteria/json.go b/model/criteria/json.go index beded9d1f..d0f453524 100644 --- a/model/criteria/json.go +++ b/model/criteria/json.go @@ -33,6 +33,20 @@ func (uc *unmarshalConjunctionType) UnmarshalJSON(data []byte) error { return nil } +// optionalConjunction is a top-level "all"/"any" value that remembers whether its +// key was present at all, so a Criteria providing both can be rejected. encoding/json +// calls UnmarshalJSON even for a JSON null, so present is set whenever the key appears +// — including as [] or null — while an absent key leaves it false. +type optionalConjunction struct { + present bool + rules unmarshalConjunctionType +} + +func (o *optionalConjunction) UnmarshalJSON(data []byte) error { + o.present = true + return json.Unmarshal(data, &o.rules) +} + func unmarshalExpression(opName string, rawValue json.RawMessage) Expression { m := make(map[string]any) err := json.Unmarshal(rawValue, &m) From be10f89c117925fabf10394b8d2962a370108b97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 11 Jul 2026 09:18:14 -0400 Subject: [PATCH 4/6] ci: don't skip release jobs after the DB migration check on tag pushes (#5760) * ci: don't skip release jobs after the DB migration check on tag pushes The validate-migrations job added in #5750 was gated at job level with if: github.event_name == 'pull_request', so it concluded "skipped" on tag pushes. GitHub Actions propagates a skipped job transitively through the needs chain (actions/runner#491): even though Build overrode its own gate with !cancelled() && !failure() and ran successfully, every job downstream of Build (msi, release, push-manifest-*, PKG uploads) still failed the implicit success() check and was skipped, which broke the v0.63.2 release. Move the pull_request gate from the job to its steps. On non-PR events all steps are skipped and the job concludes "success", so downstream jobs run normally. This also restores the default success() gate on Build, keeping the fail-fast behavior on PRs with a bad migration. * ci: trim workflow comment Condense the explanation of the step-level pull_request gate on the validate-migrations job to the essential rationale. --- .github/workflows/pipeline.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index d21d0a681..86a1055f8 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -99,16 +99,20 @@ jobs: validate-migrations: name: Validate DB migrations runs-on: ubuntu-latest - if: github.event_name == 'pull_request' + # PR-only gate is at step level: a job-level skip would propagate through + # the needs chain (actions/runner#491) and skip all release jobs on tag pushes. steps: - uses: actions/checkout@v7 + if: github.event_name == 'pull_request' with: fetch-depth: 0 # Refresh the base branch so the check compares against its CURRENT tip, # not the (possibly stale) commit the PR was opened against. - name: Fetch latest base branch + if: github.event_name == 'pull_request' run: git fetch --no-tags origin "+refs/heads/${{ github.event.pull_request.base.ref }}:refs/remotes/origin/${{ github.event.pull_request.base.ref }}" - name: Validate migration ordering and naming + if: github.event_name == 'pull_request' env: BASE_REF: origin/${{ github.event.pull_request.base.ref }} run: ./.github/workflows/validate-migrations.sh @@ -275,10 +279,6 @@ jobs: build: name: Build needs: [js, go, go-windows, go-lint, i18n-lint, git-version, check-push-enabled, validate-migrations] - # validate-migrations only runs on pull_request, so it is "skipped" on push/tag - # builds. Run Build unless a dependency actually failed — a *skipped* dependency - # (the migration check on non-PR events) must not block release builds. - if: ${{ !cancelled() && !failure() }} strategy: matrix: platform: [ linux/amd64, linux/arm64, linux/arm/v5, linux/arm/v6, linux/arm/v7, linux/386, linux/riscv64, darwin/amd64, darwin/arm64, windows/amd64, windows/386 ] From a5efba9a08e349b12cdc25f5c511080cd5f417ad Mon Sep 17 00:00:00 2001 From: Andrew Katsikas Date: Sun, 12 Jul 2026 12:25:23 -0400 Subject: [PATCH 5/6] fix(ui): Set Cache-Control: no-cache on index.html - #5766 (#5767) * fix(serve_index): Set Cache-Control: no-cache on index.html - #5766 Signed-off-by: apkatsikas * fix(serve_index): Set Cache-Control: no-cache, no-store and must-revalidate on index.html - #5766 Signed-off-by: apkatsikas --------- Signed-off-by: apkatsikas --- server/serve_index.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/serve_index.go b/server/serve_index.go index 13fa4a9ce..a538daf1a 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -107,6 +107,7 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl addShareData(r, data, shareInfo) w.Header().Set("Content-Type", "text/html") + w.Header().Set("Cache-Control", "no-store, no-cache, must-revalidate") err = t.Execute(w, data) if err != nil { log.Error(r, "Could not execute `index.html` template", err) From 6b9f85efcc42aab1156ada175a18390f5cb0fbb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sun, 12 Jul 2026 13:27:01 -0400 Subject: [PATCH 6/6] fix(subsonic): omit bit depth for lossy targets in transcode decision (#5768) getTranscodeDecision was copying the source file's bit depth into the transcodeStream details, so a 24-bit FLAC negotiated to Opus reported audioBitdepth=24. Lossy codecs (Opus, MP3, AAC) have no PCM bit depth, and ffmpeg only honors a bit depth constraint (-sample_fmt) for lossless outputs, so the value was both meaningless and misleading to clients that use it as a quality indicator. Only set the transcoded stream's bit depth when the target format is lossless; a zero value omits audioBitdepth from the response. This also makes audioBitdepth codec limitations a no-op for lossy targets instead of rejecting the profile. Lossless targets (e.g. FLAC->FLAC downconvert) keep reporting and clamping bit depth as before. --- core/stream/codec.go | 15 ++++++++----- core/stream/decider.go | 2 +- core/stream/decider_test.go | 44 ++++++++++++++++++++++++++++++++++--- 3 files changed, 51 insertions(+), 10 deletions(-) diff --git a/core/stream/codec.go b/core/stream/codec.go index 28bff75c4..56d163324 100644 --- a/core/stream/codec.go +++ b/core/stream/codec.go @@ -43,14 +43,17 @@ func normalizeSourceSampleRate(sampleRate int, codec string) int { return sampleRate } -// normalizeSourceBitDepth adjusts the source bit depth for codecs that use -// non-standard bit depths. Currently handles DSD (1-bit → 24-bit PCM, which is -// what ffmpeg produces). For other codecs, returns the depth unchanged. -func normalizeSourceBitDepth(bitDepth int, codec string) int { - if strings.EqualFold(codec, "dsd") && bitDepth == 1 { +// targetBitDepth returns the bit depth for a transcoded stream: 0 for lossy +// targets (they have no PCM bit depth), otherwise the source depth, with DSD +// adjusted to the 24-bit PCM that ffmpeg produces. +func targetBitDepth(srcBitDepth int, srcCodec string, targetIsLossless bool) int { + if !targetIsLossless { + return 0 + } + if strings.EqualFold(srcCodec, "dsd") && srcBitDepth == 1 { return 24 } - return bitDepth + return srcBitDepth } // codecFixedOutputSampleRate returns the mandatory output sample rate for codecs diff --git a/core/stream/decider.go b/core/stream/decider.go index 7940c6862..3c6b01e05 100644 --- a/core/stream/decider.go +++ b/core/stream/decider.go @@ -269,7 +269,7 @@ func (s *deciderService) computeTranscodedStream(ctx context.Context, src *Detai Codec: strings.ToLower(profile.AudioCodec), SampleRate: normalizeSourceSampleRate(src.SampleRate, src.Codec), Channels: src.Channels, - BitDepth: normalizeSourceBitDepth(src.BitDepth, src.Codec), + BitDepth: targetBitDepth(src.BitDepth, src.Codec, targetIsLossless), IsLossless: targetIsLossless, } if ts.Codec == "" { diff --git a/core/stream/decider_test.go b/core/stream/decider_test.go index 03c4ea437..577207636 100644 --- a/core/stream/decider_test.go +++ b/core/stream/decider_test.go @@ -656,6 +656,44 @@ var _ = Describe("Decider", func() { Expect(decision.TargetBitDepth).To(Equal(24)) }) + It("omits bit depth when transcoding to a lossy format", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) + ci := &ClientInfo{ + MaxTranscodingAudioBitrate: 320, + TranscodingProfiles: []Profile{ + {Container: "opus", AudioCodec: "opus", Protocol: ProtocolHTTP}, + }, + } + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TranscodeStream.BitDepth).To(BeZero()) + Expect(decision.TargetBitDepth).To(BeZero()) + }) + + It("ignores audioBitdepth limitation when transcoding to a lossy format", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) + ci := &ClientInfo{ + MaxTranscodingAudioBitrate: 320, + TranscodingProfiles: []Profile{ + {Container: "opus", AudioCodec: "opus", Protocol: ProtocolHTTP}, + }, + CodecProfiles: []CodecProfile{ + { + Type: CodecProfileTypeAudio, + Name: "opus", + Limitations: []Limitation{ + {Name: LimitationAudioBitdepth, Comparison: ComparisonGreaterThanEqual, Values: []string{"32"}, Required: true}, + }, + }, + }, + } + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TranscodeStream.BitDepth).To(BeZero()) + }) + It("rejects transcoding profile when GreaterThanEqual cannot be satisfied", func() { mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ @@ -695,9 +733,9 @@ var _ = Describe("Decider", func() { // DSD64 2822400 / 8 = 352800, capped by MP3 max of 48000 Expect(decision.TranscodeStream.SampleRate).To(Equal(48000)) Expect(decision.TargetSampleRate).To(Equal(48000)) - // DSD 1-bit → 24-bit PCM - Expect(decision.TranscodeStream.BitDepth).To(Equal(24)) - Expect(decision.TargetBitDepth).To(Equal(24)) + // MP3 is lossy: no bit depth on the transcoded stream + Expect(decision.TranscodeStream.BitDepth).To(BeZero()) + Expect(decision.TargetBitDepth).To(BeZero()) }) It("converts DSD sample rate for FLAC target without codec limit", func() {