diff --git a/docs/PLUGIN_DEVELOPMENT_GUIDE.md b/docs/PLUGIN_DEVELOPMENT_GUIDE.md index 9a3a831c..13497abb 100644 --- a/docs/PLUGIN_DEVELOPMENT_GUIDE.md +++ b/docs/PLUGIN_DEVELOPMENT_GUIDE.md @@ -654,7 +654,13 @@ Gracefully stops the plugin subprocess and removes its types from the engine. An POST /api/v1/plugins/external/{name}/reload ``` -Equivalent to unload followed by load. Useful after updating the plugin binary on disk. +Starts the replacement plugin subprocess, performs the handshake and contract +validation, then swaps it into the active slot. If the candidate fails to load, +the previously active plugin process stays registered and running. + +This API is a local activation primitive only: it consumes the plugin binary and +manifest already staged on disk. Package trust, artifact download, signature +verification, and fleet rollout policy belong to the caller or update manager. **Response (success):** ```json diff --git a/plugin/external/handler.go b/plugin/external/handler.go index 5b09fe50..e669d904 100644 --- a/plugin/external/handler.go +++ b/plugin/external/handler.go @@ -117,7 +117,8 @@ func (h *PluginHandler) handleUnload(w http.ResponseWriter, r *http.Request) { writeOK(w, map[string]string{"name": name, "action": "unloaded"}) } -// handleReload reloads an external plugin by name (unload + load). +// handleReload reloads an external plugin by name without unloading the active +// process until the replacement has loaded successfully. func (h *PluginHandler) handleReload(w http.ResponseWriter, r *http.Request) { name := r.PathValue("name") if name == "" { diff --git a/plugin/external/manager.go b/plugin/external/manager.go index b2251d91..c827f905 100644 --- a/plugin/external/manager.go +++ b/plugin/external/manager.go @@ -23,6 +23,13 @@ type ExternalPluginManager struct { clients map[string]*goplugin.Client callbackServer *CallbackServer + + startPlugin func(name string) (*pluginLaunch, error) +} + +type pluginLaunch struct { + client *goplugin.Client + adapter *ExternalPluginAdapter } // NewExternalPluginManager creates a new manager that scans the given directory for plugins. @@ -91,6 +98,25 @@ func (m *ExternalPluginManager) LoadPlugin(name string) (*ExternalPluginAdapter, return nil, fmt.Errorf("plugin %q is already loaded", name) } + launch, err := m.startPluginLocked(name) + if err != nil { + return nil, err + } + if err := validatePluginLaunch(name, launch); err != nil { + return nil, err + } + + m.clients[name] = launch.client + m.logger.Printf("plugin %q loaded successfully", name) + + return launch.adapter, nil +} + +func (m *ExternalPluginManager) startPluginLocked(name string) (*pluginLaunch, error) { + if m.startPlugin != nil { + return m.startPlugin(name) + } + pluginDir := filepath.Join(m.pluginsDir, name) manifestPath := filepath.Join(pluginDir, "plugin.json") // Resolve the binary path to absolute. os/exec.Cmd.Start evaluates a @@ -172,10 +198,7 @@ func (m *ExternalPluginManager) LoadPlugin(name string) (*ExternalPluginAdapter, return nil, fmt.Errorf("create adapter for plugin %q: %w", name, err) } - m.clients[name] = client - m.logger.Printf("plugin %q loaded successfully", name) - - return adapter, nil + return &pluginLaunch{client: client, adapter: adapter}, nil } // UnloadPlugin stops the named plugin subprocess and removes it from the internal map. @@ -196,12 +219,53 @@ func (m *ExternalPluginManager) UnloadPlugin(name string) error { return nil } -// ReloadPlugin unloads and then loads the named plugin. +// ReloadPlugin starts and validates the replacement before stopping the +// currently loaded plugin. Candidate failure leaves the old process registered. func (m *ExternalPluginManager) ReloadPlugin(name string) (*ExternalPluginAdapter, error) { - // Unload first (ignore error if not loaded) - _ = m.UnloadPlugin(name) + m.mu.Lock() + defer m.mu.Unlock() + + oldClient, wasLoaded := m.clients[name] + if !wasLoaded { + launch, err := m.startPluginLocked(name) + if err != nil { + return nil, err + } + if err := validatePluginLaunch(name, launch); err != nil { + return nil, err + } + m.clients[name] = launch.client + m.logger.Printf("plugin %q loaded successfully", name) + return launch.adapter, nil + } - return m.LoadPlugin(name) + launch, err := m.startPluginLocked(name) + if err != nil { + m.logger.Printf("plugin %q reload failed; keeping existing plugin active: %v", name, err) + return nil, fmt.Errorf("reload plugin %q: %w", name, err) + } + if err := validatePluginLaunch(name, launch); err != nil { + m.logger.Printf("plugin %q reload failed; keeping existing plugin active: %v", name, err) + return nil, fmt.Errorf("reload plugin %q: %w", name, err) + } + + m.clients[name] = launch.client + oldClient.Kill() + m.logger.Printf("plugin %q reloaded successfully", name) + return launch.adapter, nil +} + +func validatePluginLaunch(name string, launch *pluginLaunch) error { + if launch == nil { + return fmt.Errorf("plugin %q launch returned nil result", name) + } + if launch.client == nil { + return fmt.Errorf("plugin %q launch returned nil client", name) + } + if launch.adapter == nil { + return fmt.Errorf("plugin %q launch returned nil adapter", name) + } + return nil } // LoadedPlugins returns the names of all currently loaded plugins. diff --git a/plugin/external/manager_test.go b/plugin/external/manager_test.go index 6de0640b..60dcd997 100644 --- a/plugin/external/manager_test.go +++ b/plugin/external/manager_test.go @@ -1,8 +1,14 @@ package external import ( + "errors" "log" + "os" + "path/filepath" + "strings" "testing" + + goplugin "github.com/GoCodeAlone/go-plugin" ) func TestExternalPluginManagerStoresCallbackServer(t *testing.T) { @@ -15,3 +21,203 @@ func TestExternalPluginManagerStoresCallbackServer(t *testing.T) { t.Fatal("expected manager to retain callback server for plugin load") } } + +func TestExternalPluginManagerReloadFailureKeepsExistingPluginLoaded(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + oldClient := &goplugin.Client{} + manager.clients["safe-plugin"] = oldClient + manager.startPlugin = func(string) (*pluginLaunch, error) { + return nil, errors.New("candidate handshake failed") + } + + if _, err := manager.ReloadPlugin("safe-plugin"); err == nil { + t.Fatal("expected reload failure") + } + + got := manager.clients["safe-plugin"] + if got != oldClient { + t.Fatal("reload failure replaced or removed the active plugin client") + } + if !manager.IsLoaded("safe-plugin") { + t.Fatal("reload failure should leave active plugin loaded") + } +} + +func TestExternalPluginManagerLoadPluginStoresCandidateAfterValidation(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + candidate := &goplugin.Client{} + adapter := &ExternalPluginAdapter{} + manager.startPlugin = func(name string) (*pluginLaunch, error) { + if name != "safe-plugin" { + t.Fatalf("unexpected plugin name %q", name) + } + return &pluginLaunch{client: candidate, adapter: adapter}, nil + } + + got, err := manager.LoadPlugin("safe-plugin") + if err != nil { + t.Fatalf("load: %v", err) + } + + if got != adapter { + t.Fatal("load did not return candidate adapter") + } + if manager.clients["safe-plugin"] != candidate { + t.Fatal("load did not register candidate client") + } + if _, err := manager.LoadPlugin("safe-plugin"); err == nil { + t.Fatal("duplicate load should fail") + } +} + +func TestExternalPluginManagerLoadPluginRejectsInvalidCandidate(t *testing.T) { + for name, launch := range map[string]*pluginLaunch{ + "nil-launch": nil, + "nil-client": {adapter: &ExternalPluginAdapter{}}, + "nil-adapter": {client: &goplugin.Client{}}, + } { + t.Run(name, func(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + manager.startPlugin = func(string) (*pluginLaunch, error) { + return launch, nil + } + + if _, err := manager.LoadPlugin("safe-plugin"); err == nil { + t.Fatal("expected invalid candidate error") + } + if manager.IsLoaded("safe-plugin") { + t.Fatal("invalid candidate should not be registered") + } + }) + } +} + +func TestExternalPluginManagerLoadPluginReturnsStartError(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + manager.startPlugin = func(string) (*pluginLaunch, error) { + return nil, errors.New("candidate start failed") + } + + if _, err := manager.LoadPlugin("safe-plugin"); err == nil || !strings.Contains(err.Error(), "candidate start failed") { + t.Fatalf("expected start error, got %v", err) + } + if manager.IsLoaded("safe-plugin") { + t.Fatal("start failure should not register plugin") + } +} + +func TestExternalPluginManagerLoadPluginValidatesDiskCandidateBeforeStart(t *testing.T) { + pluginsDir := t.TempDir() + pluginDir := filepath.Join(pluginsDir, "safe-plugin") + if err := os.MkdirAll(filepath.Join(pluginDir, "safe-plugin"), 0o755); err != nil { + t.Fatalf("create fake binary directory: %v", err) + } + manifest := []byte(`{ + "name": "safe-plugin", + "version": "1.0.0", + "author": "test", + "description": "test plugin" + }`) + if err := os.WriteFile(filepath.Join(pluginDir, "plugin.json"), manifest, 0o644); err != nil { + t.Fatalf("write manifest: %v", err) + } + manager := NewExternalPluginManager(pluginsDir, log.Default()) + + if _, err := manager.LoadPlugin("safe-plugin"); err == nil || !strings.Contains(err.Error(), "binary path is a directory") { + t.Fatalf("expected directory binary validation error, got %v", err) + } + if manager.IsLoaded("safe-plugin") { + t.Fatal("disk validation failure should not register plugin") + } +} + +func TestExternalPluginManagerReloadWithoutActivePluginLoadsCandidate(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + candidate := &goplugin.Client{} + adapter := &ExternalPluginAdapter{} + manager.startPlugin = func(string) (*pluginLaunch, error) { + return &pluginLaunch{client: candidate, adapter: adapter}, nil + } + + got, err := manager.ReloadPlugin("safe-plugin") + if err != nil { + t.Fatalf("reload: %v", err) + } + + if got != adapter { + t.Fatal("reload did not return candidate adapter") + } + if manager.clients["safe-plugin"] != candidate { + t.Fatal("reload without active plugin did not register candidate client") + } +} + +func TestExternalPluginManagerReloadWithoutActivePluginRejectsInvalidCandidate(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + manager.startPlugin = func(string) (*pluginLaunch, error) { + return &pluginLaunch{client: &goplugin.Client{}}, nil + } + + if _, err := manager.ReloadPlugin("safe-plugin"); err == nil { + t.Fatal("expected invalid candidate error") + } + if manager.IsLoaded("safe-plugin") { + t.Fatal("invalid reload candidate should not be registered") + } +} + +func TestExternalPluginManagerReloadWithoutActivePluginReturnsStartError(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + manager.startPlugin = func(string) (*pluginLaunch, error) { + return nil, errors.New("candidate start failed") + } + + if _, err := manager.ReloadPlugin("safe-plugin"); err == nil || !strings.Contains(err.Error(), "candidate start failed") { + t.Fatalf("expected start error, got %v", err) + } + if manager.IsLoaded("safe-plugin") { + t.Fatal("start failure should not register plugin") + } +} + +func TestExternalPluginManagerReloadSuccessSwapsAfterCandidateStarts(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + oldClient := &goplugin.Client{} + newClient := &goplugin.Client{} + adapter := &ExternalPluginAdapter{} + manager.clients["safe-plugin"] = oldClient + manager.startPlugin = func(string) (*pluginLaunch, error) { + if manager.clients["safe-plugin"] != oldClient { + t.Fatal("candidate started after old plugin was removed") + } + return &pluginLaunch{client: newClient, adapter: adapter}, nil + } + + got, err := manager.ReloadPlugin("safe-plugin") + if err != nil { + t.Fatalf("reload: %v", err) + } + + if got != adapter { + t.Fatal("reload did not return candidate adapter") + } + if got := manager.clients["safe-plugin"]; got != newClient { + t.Fatal("reload success did not register candidate plugin client") + } +} + +func TestExternalPluginManagerReloadLoadedRejectsInvalidCandidate(t *testing.T) { + manager := NewExternalPluginManager(t.TempDir(), log.Default()) + oldClient := &goplugin.Client{} + manager.clients["safe-plugin"] = oldClient + manager.startPlugin = func(string) (*pluginLaunch, error) { + return &pluginLaunch{client: &goplugin.Client{}}, nil + } + + if _, err := manager.ReloadPlugin("safe-plugin"); err == nil { + t.Fatal("expected invalid candidate error") + } + if manager.clients["safe-plugin"] != oldClient { + t.Fatal("invalid reload candidate replaced active plugin") + } +}