Merge pull request #87 from smart-mcp-proxy/bugfix/config-port

Fix: Save config to disk when restart required (fixes listen address changes)

Author: Dumbris

internal/runtime/apply_config_restart_test.go

@@ -0,0 +1,145 @@
+package runtime
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"go.uber.org/zap"
+
+	"mcpproxy-go/internal/config"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestApplyConfig_ListenAddressChange tests that listen address changes are saved to disk
+// even though they require a restart
+func TestApplyConfig_ListenAddressChange(t *testing.T) {
+	// Create temp directory and config file
+	tmpDir := t.TempDir()
+	cfgPath := filepath.Join(tmpDir, "config.json")
+
+	// Initial config with port 8080
+	initialCfg := config.DefaultConfig()
+	initialCfg.Listen = "127.0.0.1:8080"
+	initialCfg.DataDir = tmpDir
+
+	// Save initial config
+	err := config.SaveConfig(initialCfg, cfgPath)
+	require.NoError(t, err)
+
+	// Create runtime with initial config
+	rt, err := New(initialCfg, cfgPath, zap.NewNop())
+	require.NoError(t, err)
+	defer func() {
+		_ = rt.Close()
+	}()
+
+	// Create new config with different listen address
+	newCfg := config.DefaultConfig()
+	newCfg.Listen = "127.0.0.1:30080" // Changed port
+	newCfg.DataDir = tmpDir
+
+	// Apply the new config
+	result, err := rt.ApplyConfig(newCfg, cfgPath)
+	require.NoError(t, err)
+	require.NotNil(t, result)
+
+	// Verify that restart is required (listen address changes require restart)
+	assert.True(t, result.RequiresRestart, "Listen address change should require restart")
+	assert.Contains(t, result.ChangedFields, "listen", "Should detect listen address change")
+
+	// The critical test: verify config was saved to disk despite requiring restart
+	savedCfg, err := config.LoadFromFile(cfgPath)
+	require.NoError(t, err)
+
+	assert.Equal(t, "127.0.0.1:30080", savedCfg.Listen,
+		"Config file should be updated with new listen address even though restart is required")
+}
+
+// TestApplyConfig_HotReloadableChange tests that hot-reloadable changes work correctly
+func TestApplyConfig_HotReloadableChange(t *testing.T) {
+	// Create temp directory and config file
+	tmpDir := t.TempDir()
+	cfgPath := filepath.Join(tmpDir, "config.json")
+
+	// Initial config
+	initialCfg := config.DefaultConfig()
+	initialCfg.Listen = "127.0.0.1:8080"
+	initialCfg.DataDir = tmpDir
+	initialCfg.TopK = 5
+
+	// Save initial config
+	err := config.SaveConfig(initialCfg, cfgPath)
+	require.NoError(t, err)
+
+	// Create runtime
+	rt, err := New(initialCfg, cfgPath, zap.NewNop())
+	require.NoError(t, err)
+	defer func() {
+		_ = rt.Close()
+	}()
+
+	// Create new config with different TopK (hot-reloadable)
+	newCfg := config.DefaultConfig()
+	newCfg.Listen = "127.0.0.1:8080" // Same listen address
+	newCfg.DataDir = tmpDir
+	newCfg.TopK = 10 // Changed TopK
+
+	// Apply the new config
+	result, err := rt.ApplyConfig(newCfg, cfgPath)
+	require.NoError(t, err)
+	require.NotNil(t, result)
+
+	// Verify that restart is NOT required (TopK is hot-reloadable)
+	assert.False(t, result.RequiresRestart, "TopK change should not require restart")
+	assert.True(t, result.AppliedImmediately, "TopK change should be applied immediately")
+
+	// Verify config was saved to disk
+	savedCfg, err := config.LoadFromFile(cfgPath)
+	require.NoError(t, err)
+
+	assert.Equal(t, 10, savedCfg.TopK, "Config file should be updated with new TopK value")
+}
+
+// TestApplyConfig_SaveFailure tests handling of save errors
+func TestApplyConfig_SaveFailure(t *testing.T) {
+	// Create temp directory and config file
+	tmpDir := t.TempDir()
+	cfgPath := filepath.Join(tmpDir, "config.json")
+
+	// Initial config
+	initialCfg := config.DefaultConfig()
+	initialCfg.Listen = "127.0.0.1:8080"
+	initialCfg.DataDir = tmpDir
+
+	// Save initial config
+	err := config.SaveConfig(initialCfg, cfgPath)
+	require.NoError(t, err)
+
+	// Create runtime
+	rt, err := New(initialCfg, cfgPath, zap.NewNop())
+	require.NoError(t, err)
+	defer func() {
+		_ = rt.Close()
+	}()
+
+	// Make config file read-only to force save failure
+	err = os.Chmod(cfgPath, 0444)
+	require.NoError(t, err)
+	defer func() {
+		_ = os.Chmod(cfgPath, 0644)
+	}()
+
+	// Create new config
+	newCfg := config.DefaultConfig()
+	newCfg.Listen = "127.0.0.1:30080"
+	newCfg.DataDir = tmpDir
+
+	// Apply should fail because config can't be saved
+	result, err := rt.ApplyConfig(newCfg, cfgPath)
+	assert.Error(t, err, "Should fail when config cannot be saved")
+	assert.NotNil(t, result)
+	assert.False(t, result.Success, "Result should indicate failure")
+}

internal/runtime/runtime.go

@@ -813,7 +813,27 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp
 		return result, fmt.Errorf("failed to detect config changes")
 	}
 
-	// If restart is required, don't apply changes (let user restart)
+	// Save configuration to disk BEFORE checking if restart is required
+	// This ensures config changes that require restart are persisted and take effect on next startup
+	savePath := cfgPath
+	if savePath == "" {
+		savePath = r.cfgPath
+	}
+	saveErr := config.SaveConfig(newCfg, savePath)
+	if saveErr != nil {
+		r.logger.Error("Failed to save configuration to disk",
+			zap.String("path", savePath),
+			zap.Error(saveErr))
+		r.mu.Unlock() // Unlock before returning
+		return &ConfigApplyResult{
+			Success: false,
+		}, fmt.Errorf("failed to save configuration: %w", saveErr)
+	} else {
+		r.logger.Info("Configuration successfully saved to disk",
+			zap.String("path", savePath))
+	}
+
+	// If restart is required, don't apply changes in-memory (let user restart)
 	if result.RequiresRestart {
 		r.logger.Warn("Configuration changes require restart",
 			zap.String("reason", result.RestartReason),
@@ -829,19 +849,6 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp
 		r.cfgPath = cfgPath
 	}
 
-	// Save configuration to disk
-	saveErr := config.SaveConfig(newCfg, r.cfgPath)
-	if saveErr != nil {
-		r.logger.Error("Failed to save configuration to disk",
-			zap.String("path", r.cfgPath),
-			zap.Error(saveErr))
-		// Don't fail the entire operation, but log the error
-		// In-memory changes are still applied
-	} else {
-		r.logger.Info("Configuration successfully saved to disk",
-			zap.String("path", r.cfgPath))
-	}
-
 	// Apply configuration changes to components
 	r.logger.Info("Applying configuration hot-reload",
 		zap.Strings("changed_fields", result.ChangedFields))