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") }) + }) + }) +})