fix(plugin): keep active plugin on reload failure (#680)

* fix(plugin): keep active plugin on reload failure

* test(plugin): cover safe reload branches

Reject nil reload adapters and expand manager reload/load tests for PR #680 patch coverage.

Author: intel352

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

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 == "" {

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.

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