From 0fa090261d751d00dc25e968c01cf3ac03f423d8 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 15 Jul 2026 21:27:11 +0300 Subject: [PATCH 01/12] fix(runtime): add config file watcher so external edits hot-reload (HOTRELOAD) The config file watcher advertised by CLAUDE.md and the swagger docs never existed: fsnotify was only an indirect dependency and nothing in the code watched mcp_config.json. External edits (editor saves, jq > tmp && mv, CLI commands against a running core) silently never took effect until restart, while REST PATCH worked because it goes through ApplyConfig. Add internal/runtime/config_watcher.go: an fsnotify watcher on the config file's parent directory (rename/recreate-safe), started from StartBackgroundInitialization and tied to the runtime app context. Events for the config file are debounced 500ms and funneled into the existing canonical disk-reload path Runtime.ReloadConfiguration (events, telemetry NotifyConfigChanged, update-check re-gating, ConnectAll + reindex included). Safety properties: - Self-write suppression: disk bytes are compared against the marshaled in-memory snapshot, so mcpproxy's own ApplyConfig/SaveConfiguration writes never echo back into a redundant reload. - Invalid JSON keeps the previous configuration (ReloadFromFile fails before mutating state) and the watcher survives to pick up the next valid write. - Watcher startup failure degrades gracefully to the previous no-watcher behavior with a warning. Promote fsnotify v1.9.0 from indirect to direct in go.mod (already in go.sum, no new third-party code). Update stale comments that documented the watcher's absence, and document debounce/invalid-JSON/restart-required semantics in docs/configuration.md. Known limitations: a config-path change after startup is not re-watched; restart-required fields edited externally reload into memory but still need a restart to take effect (same semantics as manual reload). --- cmd/mcpproxy/telemetry_cmd.go | 8 +- docs/configuration.md | 9 + go.mod | 2 +- internal/runtime/config_watcher.go | 139 +++++++++++++++ internal/runtime/config_watcher_test.go | 216 ++++++++++++++++++++++++ internal/runtime/lifecycle.go | 21 ++- 6 files changed, 384 insertions(+), 11 deletions(-) create mode 100644 internal/runtime/config_watcher.go create mode 100644 internal/runtime/config_watcher_test.go diff --git a/cmd/mcpproxy/telemetry_cmd.go b/cmd/mcpproxy/telemetry_cmd.go index 8623c6d3b..e3b392a74 100644 --- a/cmd/mcpproxy/telemetry_cmd.go +++ b/cmd/mcpproxy/telemetry_cmd.go @@ -290,9 +290,11 @@ func runTelemetryDisable(cmd *cobra.Command, _ []string) error { // command never appears to hang on the (best-effort) beacon below. fmt.Println("Telemetry disabled.") - // One-time opt-out beacon. When a daemon is running it does NOT auto-reload - // this file (there is no fsnotify watcher), so the CLI is responsible for the - // beacon in the CLI-driven path. Route it through the SAME guarded server-side + // One-time opt-out beacon. A running daemon now hot-reloads this file via + // the fsnotify config watcher (config_watcher.go) and fires its own beacon + // through NotifyConfigChanged — but no daemon may be running, so the CLI + // stays responsible for the beacon in the CLI-driven path. Route it + // through the SAME guarded server-side // entry point (EmitOptOutBeacon applies the dev-build/semver, env, and // anon-id guards and owns the single send) rather than duplicating the send // or bypassing a guard. A short timeout keeps this from blocking on a slow diff --git a/docs/configuration.md b/docs/configuration.md index b52a6ee53..88c8162ea 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -34,6 +34,15 @@ MCPProxy looks for configuration in these locations (in order): **Note:** At first launch, MCPProxy automatically generates a minimal configuration file if none exists. +### Hot-Reload on File Edits + +A running MCPProxy core watches `mcp_config.json` and hot-reloads external edits automatically — whether written in place (`echo ... > mcp_config.json`) or atomically (`jq ... > tmp && mv tmp mcp_config.json`, the pattern most editors use). Behavior details: + +- Edits are debounced for ~500 ms, so rapid write bursts collapse into a single reload. +- Invalid JSON is rejected safely: the running configuration is kept unchanged (a warning is logged) and the watcher picks up the next valid write. +- MCPProxy's own saves (Web UI, REST `PATCH /api/v1/config`, CLI commands) do not trigger a redundant second reload. +- Restart-required fields (e.g. `listen`, `data_dir`) are reloaded into memory but only take effect after a restart. + --- ## Basic Configuration diff --git a/go.mod b/go.mod index 786c32fb4..d31c867a9 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/denisbrodbeck/machineid v1.0.1 github.com/dop251/goja v0.0.0-20260305124333-6a7976c22267 github.com/evanw/esbuild v0.28.1 + github.com/fsnotify/fsnotify v1.9.0 github.com/gen2brain/beeep v0.11.2 github.com/go-chi/chi/v5 v5.3.1 github.com/golang-jwt/jwt/v5 v5.3.1 @@ -86,7 +87,6 @@ require ( github.com/dlclark/regexp2 v1.11.4 // indirect github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect github.com/esiqveland/notify v0.13.3 // indirect - github.com/fsnotify/fsnotify v1.9.0 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go new file mode 100644 index 000000000..34b04b953 --- /dev/null +++ b/internal/runtime/config_watcher.go @@ -0,0 +1,139 @@ +package runtime + +import ( + "bytes" + "context" + "encoding/json" + "os" + "path/filepath" + "time" + + "github.com/fsnotify/fsnotify" + "go.uber.org/zap" +) + +// configWatchDebounce coalesces bursts of file events (editor write patterns, +// tmp-file + rename sequences) into a single reload, and gives truncate-write +// editors time to finish writing before we read the file. +const configWatchDebounce = 500 * time.Millisecond + +// startConfigFileWatcher watches the config file for external edits and +// hot-reloads them through Runtime.ReloadConfiguration — the same canonical +// disk-reload path used by manual reloads (events, telemetry and update-check +// hooks included). +// +// It watches the parent directory rather than the file itself so that +// rename/recreate writes (`mv tmp file`, atomic-write editors, and mcpproxy's +// own atomicWriteFile) can never orphan the watch. The directory watch is +// added synchronously so callers (and tests) have no startup race; only the +// event loop runs in a goroutine, tied to ctx. +// +// On failure the watcher degrades gracefully: a warning is logged, an error is +// returned, and the server keeps running without hot-reload. +func (r *Runtime) startConfigFileWatcher(ctx context.Context, cfgPath string) error { + absPath, err := filepath.Abs(cfgPath) + if err != nil { + r.logger.Warn("Config file watcher unavailable; external edits will not hot-reload", + zap.String("path", cfgPath), zap.Error(err)) + return err + } + + watcher, err := fsnotify.NewWatcher() + if err != nil { + r.logger.Warn("Config file watcher unavailable; external edits will not hot-reload", + zap.String("path", absPath), zap.Error(err)) + return err + } + + if err := watcher.Add(filepath.Dir(absPath)); err != nil { + _ = watcher.Close() + r.logger.Warn("Config file watcher unavailable; external edits will not hot-reload", + zap.String("path", absPath), zap.Error(err)) + return err + } + + r.logger.Info("Config file watcher started", zap.String("path", absPath)) + go r.runConfigWatchLoop(ctx, watcher, absPath) + return nil +} + +// runConfigWatchLoop is the watcher event loop: it filters events down to the +// config file, debounces them, and triggers reloadFromDiskIfChanged when the +// debounce window closes. +func (r *Runtime) runConfigWatchLoop(ctx context.Context, watcher *fsnotify.Watcher, absPath string) { + defer func() { _ = watcher.Close() }() + + // Debounce timer, created stopped. Go 1.23+ timer semantics make + // Stop/Reset race-free without channel draining. + debounce := time.NewTimer(configWatchDebounce) + if !debounce.Stop() { + <-debounce.C + } + defer debounce.Stop() + + // Create catches mv/rename-in; Write catches in-place truncate writes; + // Rename/Remove re-arm the debounce so the follow-up recreate isn't + // missed. Chmod is deliberately ignored. + const relevantOps = fsnotify.Create | fsnotify.Write | fsnotify.Rename | fsnotify.Remove + + for { + select { + case <-ctx.Done(): + return + + case err, ok := <-watcher.Errors: + if !ok { + return + } + r.logger.Warn("Config file watcher error", zap.Error(err)) + + case ev, ok := <-watcher.Events: + if !ok { + return + } + if filepath.Clean(ev.Name) != absPath || ev.Op&relevantOps == 0 { + continue + } + debounce.Reset(configWatchDebounce) + + case <-debounce.C: + r.reloadFromDiskIfChanged(absPath) + } + } +} + +// reloadFromDiskIfChanged reloads the configuration from disk unless the file +// content matches the in-memory snapshot (self-write suppression: our own +// ApplyConfig / SaveConfiguration writes must not echo back into a redundant +// reload, which would re-trigger ConnectAll + reindex churn). +func (r *Runtime) reloadFromDiskIfChanged(absPath string) { + diskBytes, err := os.ReadFile(absPath) + if err != nil { + // Rename window — the file may be briefly absent; the follow-up + // Create event re-arms the debounce and we retry then. + r.logger.Debug("Config file not readable after change event; waiting for next event", + zap.String("path", absPath), zap.Error(err)) + return + } + + // Self-write suppression: marshal the current snapshot exactly as + // config.SaveConfig does and byte-compare with disk. Equal bytes mean the + // event came from our own save. If a future save path diverges, this + // degrades to one redundant (idempotent) reload — never a loop, since + // ReloadConfiguration never writes the file. + if current, merr := json.MarshalIndent(r.ConfigSnapshot().Config, "", " "); merr == nil { + if bytes.Equal(bytes.TrimSpace(current), bytes.TrimSpace(diskBytes)) { + r.logger.Debug("Config file event matches in-memory config; skipping reload", + zap.String("path", absPath)) + return + } + } + + r.logger.Info("Config file changed on disk, hot-reloading", zap.String("path", absPath)) + if err := r.ReloadConfiguration(); err != nil { + // ReloadFromFile returns before mutating state on parse/validation + // failure, so a bad file leaves the previous config intact. + r.logger.Warn("Config hot-reload failed; keeping previous configuration", + zap.String("path", absPath), zap.Error(err)) + } +} diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go new file mode 100644 index 000000000..a3f5a9738 --- /dev/null +++ b/internal/runtime/config_watcher_test.go @@ -0,0 +1,216 @@ +package runtime + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/configsvc" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newWatcherTestRuntime builds a Runtime backed by a real config file in a +// temp dir and starts the config file watcher on it. Same construction +// pattern as restart_disk_reload_test.go. +func newWatcherTestRuntime(t *testing.T) (*Runtime, *config.Config, string) { + t.Helper() + + tmpDir := t.TempDir() + cfgPath := filepath.Join(tmpDir, "mcp_config.json") + + initialCfg := config.DefaultConfig() + initialCfg.Listen = "127.0.0.1:0" + initialCfg.DataDir = tmpDir + require.NoError(t, config.SaveConfig(initialCfg, cfgPath)) + + rt, err := New(initialCfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + + require.NoError(t, rt.startConfigFileWatcher(rt.AppContext(), cfgPath)) + + return rt, initialCfg, cfgPath +} + +// editedConfig returns a copy-ish config with a distinct hot-reloadable value +// so tests can assert the reload landed. +func editedConfig(base *config.Config, limit int) *config.Config { + edited := config.DefaultConfig() + edited.Listen = base.Listen + edited.DataDir = base.DataDir + edited.ToolResponseLimit = limit + return edited +} + +// TestConfigWatcher_RenameWriteTriggersReload covers the atomic-write style +// (`jq ... > tmp && mv tmp mcp_config.json`) that editors and mcpproxy's own +// SaveConfig use: write to a temp file, then rename over the config path. +func TestConfigWatcher_RenameWriteTriggersReload(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + // config.SaveConfig itself is an atomic tmp-file + rename write. + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 12345), cfgPath)) + + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 12345 + }, 5*time.Second, 25*time.Millisecond, + "external rename-style config edit must hot-reload into the running core") +} + +// TestConfigWatcher_TruncateWriteTriggersReload covers the in-place +// truncate-write style (`echo ... > mcp_config.json`). +func TestConfigWatcher_TruncateWriteTriggersReload(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + data, err := os.ReadFile(cfgPath) + require.NoError(t, err) + require.NotEmpty(t, data) + + edited := editedConfig(initialCfg, 23456) + require.NoError(t, config.SaveConfig(edited, cfgPath+".staging")) + newBytes, err := os.ReadFile(cfgPath + ".staging") + require.NoError(t, err) + require.NoError(t, os.WriteFile(cfgPath, newBytes, 0o600)) + + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 23456 + }, 5*time.Second, 25*time.Millisecond, + "external in-place config edit must hot-reload into the running core") +} + +// TestConfigWatcher_InvalidJSONKeepsOldConfigThenRecovers: a broken write must +// not clobber the running config, and the watcher must survive it and pick up +// the next valid write. +func TestConfigWatcher_InvalidJSONKeepsOldConfigThenRecovers(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + versionBefore := rt.ConfigSnapshot().Version + limitBefore := rt.ConfigSnapshot().Config.ToolResponseLimit + + require.NoError(t, os.WriteFile(cfgPath, []byte("{not valid json"), 0o600)) + + // Give the debounce + reload attempt time to run, then confirm nothing changed. + time.Sleep(1500 * time.Millisecond) + assert.Equal(t, limitBefore, rt.ConfigSnapshot().Config.ToolResponseLimit, + "invalid JSON must keep the previous configuration") + assert.Equal(t, versionBefore, rt.ConfigSnapshot().Version, + "invalid JSON must not bump the config version") + + // Watcher must still be alive: a valid write now reloads. + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 34567), cfgPath)) + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 34567 + }, 5*time.Second, 25*time.Millisecond, + "watcher must recover after a failed reload") +} + +// TestConfigWatcher_SelfWriteSuppressed: mcpproxy's own ApplyConfig saves the +// file to disk; the watcher must not echo that save back into a second +// disk-reload (ConnectAll + reindex churn). +func TestConfigWatcher_SelfWriteSuppressed(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + updates := rt.ConfigService().Subscribe(ctx) + defer rt.ConfigService().Unsubscribe(updates) + + result, err := rt.ApplyConfig(editedConfig(initialCfg, 45678), cfgPath) + require.NoError(t, err) + require.True(t, result.Success) + + // Drain the expected in-memory apply update (plus the initial snapshot). + deadline := time.After(5 * time.Second) + sawApply := false + for !sawApply { + select { + case u := <-updates: + if u.Source == "api_apply_config" { + sawApply = true + } + case <-deadline: + t.Fatal("never saw the api_apply_config update") + } + } + + // Now assert the watcher does NOT follow up with a file_reload echo. + timeout := time.After(1500 * time.Millisecond) + for { + select { + case u := <-updates: + assert.NotEqual(t, configsvc.UpdateTypeReload, u.Type, + "watcher must not echo mcpproxy's own config save back as a disk reload (source=%s)", u.Source) + case <-timeout: + return + } + } +} + +// TestConfigWatcher_DebounceCoalesces: a burst of rapid writes must collapse +// into one (or at most a couple of) reloads, not one per write. +func TestConfigWatcher_DebounceCoalesces(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + updates := rt.ConfigService().Subscribe(ctx) + defer rt.ConfigService().Unsubscribe(updates) + + for i := 1; i <= 10; i++ { + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 50000+i), cfgPath)) + time.Sleep(20 * time.Millisecond) + } + + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 50010 + }, 5*time.Second, 25*time.Millisecond, "final write must land") + + // Allow any straggler debounce window to fire, then count reloads. + time.Sleep(1 * time.Second) + reloads := 0 + for { + select { + case u := <-updates: + if u.Type == configsvc.UpdateTypeReload { + reloads++ + } + continue + default: + } + break + } + assert.LessOrEqual(t, reloads, 3, + "10 rapid writes must be debounced into a few reloads, got %d", reloads) + assert.GreaterOrEqual(t, reloads, 1, "at least one reload must have happened") +} + +// TestConfigWatcher_MissingDirGracefulDegradation: a watch path whose parent +// directory doesn't exist must fail gracefully (error, no panic) and leave the +// runtime usable. +func TestConfigWatcher_MissingDirGracefulDegradation(t *testing.T) { + tmpDir := t.TempDir() + cfgPath := filepath.Join(tmpDir, "mcp_config.json") + + initialCfg := config.DefaultConfig() + initialCfg.Listen = "127.0.0.1:0" + initialCfg.DataDir = tmpDir + require.NoError(t, config.SaveConfig(initialCfg, cfgPath)) + + rt, err := New(initialCfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + defer func() { _ = rt.Close() }() + + bogus := filepath.Join(tmpDir, "does", "not", "exist", "mcp_config.json") + err = rt.startConfigFileWatcher(rt.AppContext(), bogus) + assert.Error(t, err, "watching a nonexistent directory must return an error") + + // Runtime still works. + assert.NotNil(t, rt.ConfigSnapshot()) +} diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index f1ec0ffd6..35161d6e6 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -134,6 +134,13 @@ func (r *Runtime) StartBackgroundInitialization() { r.logger.Info("Tool discovery callback registered on upstream manager") } + // Watch the config file for external edits (editors, CLI, `jq > tmp && mv`) + // and hot-reload them through the canonical disk-reload path. Failure + // degrades gracefully to no hot-reload (warning logged inside). + if p := r.ConfigSnapshot().Path; p != "" { + _ = r.startConfigFileWatcher(r.appCtx, p) + } + go r.backgroundInitialization() } @@ -1114,8 +1121,9 @@ func (r *Runtime) ReloadConfiguration() error { // MCP-2482: detect a telemetry enabled->disabled flip across the reload and // fire the one-time opt-out beacon. This covers config changes that arrive - // via a disk reload (there is no fsnotify auto-watcher, so this is the - // manual/triggered-reload path). nil-safe + fire-and-forget. + // via a disk reload — both the manual/triggered-reload path and the + // fsnotify config file watcher (config_watcher.go), which funnels external + // file edits into this method. nil-safe + fire-and-forget. if r.telemetryService != nil { r.telemetryService.NotifyConfigChanged(newSnapshot.Config) } @@ -1423,11 +1431,10 @@ func (r *Runtime) RestartServer(serverName string) error { r.logger.Info("Request to restart server", zap.String("server", serverName)) // Issue #467: pull the latest server config from disk before falling - // back to BoltDB. There is no fsnotify-style auto file-watcher, so a - // user who edits mcp_config.json and then triggers a restart would - // otherwise replay stale env / headers / args / isolation data — only - // the live REST PATCH path used to update them. Disk-first here closes - // that gap for the (much more common) edit-then-restart UX. + // back to BoltDB. The fsnotify config file watcher (config_watcher.go) + // now hot-reloads external edits, but its debounce window means a fast + // edit-then-restart could still race a stale BoltDB record — disk-first + // here keeps the edit-then-restart UX deterministic regardless. serverConfig := r.lookupServerConfigForRestart(serverName) if serverConfig == nil { return fmt.Errorf("server '%s' not found in configuration", serverName) From 58c6335691cfba9f8fc88ec9cc3762151dcb722e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 15 Jul 2026 22:01:15 +0300 Subject: [PATCH 02/12] fix(runtime): suppress watcher echo of restart-required ApplyConfig saves A restart-required ApplyConfig (listen/data_dir/api_key/TLS change) saves the new config to disk but intentionally leaves memory on the old one (requires_restart contract). The watcher's snapshot-based self-write suppression compared disk against memory, misread that save as an external edit, and hot-applied the deferred config ~500ms after the API reported applied_immediately=false. Record the marshaled bytes of every ApplyConfig save (noteConfigSelfWrite) and suppress watcher events whose disk content matches the last self-write, on top of the existing snapshot comparison. Found by cross-model review. --- internal/runtime/config_watcher.go | 48 ++++++++++++++++++++++--- internal/runtime/config_watcher_test.go | 38 ++++++++++++++++++++ internal/runtime/runtime.go | 16 +++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index 34b04b953..263011d15 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -10,6 +10,8 @@ import ( "github.com/fsnotify/fsnotify" "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" ) // configWatchDebounce coalesces bursts of file events (editor write patterns, @@ -102,10 +104,38 @@ func (r *Runtime) runConfigWatchLoop(ctx context.Context, watcher *fsnotify.Watc } } +// noteConfigSelfWrite records the marshaled form of a config mcpproxy itself +// just saved to disk, so the watcher can suppress the echo even when the +// in-memory snapshot intentionally diverges from the file — the +// restart-required ApplyConfig path saves to disk but defers the in-memory +// apply until restart (`requires_restart` contract), and the watcher must not +// hot-apply it behind the API's back. Marshals exactly as config.SaveConfig +// does; a marshal failure just skips recording (the write itself would have +// failed the same way). +func (r *Runtime) noteConfigSelfWrite(cfg *config.Config) { + data, err := json.MarshalIndent(cfg, "", " ") + if err != nil { + return + } + r.selfWriteMu.Lock() + r.lastSelfWrite = bytes.TrimSpace(data) + r.selfWriteMu.Unlock() +} + +// matchesLastSelfWrite reports whether trimmed disk bytes equal the last +// config mcpproxy itself saved (see noteConfigSelfWrite). +func (r *Runtime) matchesLastSelfWrite(trimmedDisk []byte) bool { + r.selfWriteMu.Lock() + defer r.selfWriteMu.Unlock() + return len(r.lastSelfWrite) > 0 && bytes.Equal(r.lastSelfWrite, trimmedDisk) +} + // reloadFromDiskIfChanged reloads the configuration from disk unless the file -// content matches the in-memory snapshot (self-write suppression: our own -// ApplyConfig / SaveConfiguration writes must not echo back into a redundant -// reload, which would re-trigger ConnectAll + reindex churn). +// content matches the in-memory snapshot or the last self-written config +// (self-write suppression: our own ApplyConfig / SaveConfiguration writes +// must not echo back into a redundant reload, which would re-trigger +// ConnectAll + reindex churn — or, for restart-required applies, hot-apply a +// change the API just reported as deferred until restart). func (r *Runtime) reloadFromDiskIfChanged(absPath string) { diskBytes, err := os.ReadFile(absPath) if err != nil { @@ -121,14 +151,24 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { // event came from our own save. If a future save path diverges, this // degrades to one redundant (idempotent) reload — never a loop, since // ReloadConfiguration never writes the file. + trimmedDisk := bytes.TrimSpace(diskBytes) if current, merr := json.MarshalIndent(r.ConfigSnapshot().Config, "", " "); merr == nil { - if bytes.Equal(bytes.TrimSpace(current), bytes.TrimSpace(diskBytes)) { + if bytes.Equal(bytes.TrimSpace(current), trimmedDisk) { r.logger.Debug("Config file event matches in-memory config; skipping reload", zap.String("path", absPath)) return } } + // Restart-required applies save to disk without touching memory, so the + // snapshot comparison above can't recognize them as ours — check the + // recorded last self-write too. + if r.matchesLastSelfWrite(trimmedDisk) { + r.logger.Debug("Config file event matches mcpproxy's own last save; skipping reload", + zap.String("path", absPath)) + return + } + r.logger.Info("Config file changed on disk, hot-reloading", zap.String("path", absPath)) if err := r.ReloadConfiguration(); err != nil { // ReloadFromFile returns before mutating state on parse/validation diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index a3f5a9738..80eb356bc 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -153,6 +153,44 @@ func TestConfigWatcher_SelfWriteSuppressed(t *testing.T) { } } +// TestConfigWatcher_RestartRequiredApplyNotEchoed: a restart-required +// ApplyConfig (e.g. `listen` change) saves the new config to disk but +// intentionally does NOT apply it in-memory (`requires_restart=true`, +// `applied_immediately=false`). The watcher must not treat that self-write as +// an external edit — otherwise it would hot-apply the change ~500ms after the +// API just promised it was deferred until restart. +func TestConfigWatcher_RestartRequiredApplyNotEchoed(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + updates := rt.ConfigService().Subscribe(ctx) + defer rt.ConfigService().Unsubscribe(updates) + + limitBefore := rt.ConfigSnapshot().Config.ToolResponseLimit + + edited := editedConfig(initialCfg, 56789) + edited.Listen = "127.0.0.1:1" // restart-required field + result, err := rt.ApplyConfig(edited, cfgPath) + require.NoError(t, err) + require.True(t, result.RequiresRestart, "listen change must require restart") + + // Give the watcher debounce ample time to fire, then assert the deferred + // config was NOT hot-applied behind the API's back. + timeout := time.After(1500 * time.Millisecond) + for { + select { + case u := <-updates: + assert.NotEqual(t, configsvc.UpdateTypeReload, u.Type, + "watcher must not echo a restart-required self-save back as a disk reload (source=%s)", u.Source) + case <-timeout: + assert.Equal(t, limitBefore, rt.ConfigSnapshot().Config.ToolResponseLimit, + "restart-required apply must stay deferred; watcher must not hot-apply it") + return + } + } +} + // TestConfigWatcher_DebounceCoalesces: a burst of rapid writes must collapse // into one (or at most a couple of) reloads, not one per write. func TestConfigWatcher_DebounceCoalesces(t *testing.T) { diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 82e6b4a6a..b85a8cea2 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -63,6 +63,15 @@ type Runtime struct { mu sync.RWMutex running bool + // Config-watcher self-write suppression (config_watcher.go): marshaled + // bytes of the last config mcpproxy itself saved to disk. Needed on top + // of the snapshot comparison because a restart-required ApplyConfig saves + // the new config to disk while intentionally leaving memory on the old + // one (requires_restart contract) — the snapshot alone can't identify + // that write as ours. + selfWriteMu sync.Mutex + lastSelfWrite []byte + statusMu sync.RWMutex status Status statusCh chan Status @@ -1378,6 +1387,13 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp zap.String("path", savePath)) } + // Record the save for the config watcher's self-write suppression. This + // matters most for the restart-required branch below: disk gets the new + // config while memory intentionally keeps the old one, so the watcher's + // snapshot comparison alone would misread our own save as an external + // edit and hot-apply it behind the API's back (config_watcher.go). + r.noteConfigSelfWrite(newCfg) + // If restart is required, don't apply changes in-memory (let user restart) if result.RequiresRestart { r.logger.Warn("Configuration changes require restart", From 381538b47451a33e4de8cddcda5b56713c3b3c7b Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 15 Jul 2026 22:09:02 +0300 Subject: [PATCH 03/12] fix(runtime): drop stale self-write record on genuine external reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recorded ApplyConfig self-write was never invalidated, so after an external edit B hot-reloaded, a later external revert to byte-identical last-saved bytes (editor undo, git checkout) matched the stale record and was suppressed — the running config silently stayed on B until restart. Clear the record whenever a genuine external change proceeds to reload; a suppression match itself keeps it armed so the restart-required pending state still suppresses repeat events. Found by cross-model review. --- internal/runtime/config_watcher.go | 17 ++++++++++++++ internal/runtime/config_watcher_test.go | 30 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index 263011d15..3a9d74dba 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -130,6 +130,18 @@ func (r *Runtime) matchesLastSelfWrite(trimmedDisk []byte) bool { return len(r.lastSelfWrite) > 0 && bytes.Equal(r.lastSelfWrite, trimmedDisk) } +// clearLastSelfWrite drops the recorded self-write. Called when a genuine +// external change is about to reload: from that point the file's history has +// diverged from our last save, and keeping the stale record would suppress a +// later external revert to those exact bytes (editor undo, `git checkout`). +// It is NOT cleared on a suppression match itself, so the restart-required +// pending state keeps suppressing repeat events for the same bytes. +func (r *Runtime) clearLastSelfWrite() { + r.selfWriteMu.Lock() + r.lastSelfWrite = nil + r.selfWriteMu.Unlock() +} + // reloadFromDiskIfChanged reloads the configuration from disk unless the file // content matches the in-memory snapshot or the last self-written config // (self-write suppression: our own ApplyConfig / SaveConfiguration writes @@ -169,6 +181,11 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { return } + // Genuine external change: the file no longer descends from our last + // save, so the recorded self-write is stale — drop it (see + // clearLastSelfWrite for the revert scenario it would otherwise break). + r.clearLastSelfWrite() + r.logger.Info("Config file changed on disk, hot-reloading", zap.String("path", absPath)) if err := r.ReloadConfiguration(); err != nil { // ReloadFromFile returns before mutating state on parse/validation diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index 80eb356bc..f1737dd05 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -191,6 +191,36 @@ func TestConfigWatcher_RestartRequiredApplyNotEchoed(t *testing.T) { } } +// TestConfigWatcher_ExternalRevertToLastSelfWriteReloads: the recorded +// self-write must not outlive the next genuine external reload. Sequence: +// ApplyConfig saves A (recorded), external edit B hot-reloads, then the user +// reverts the file to byte-identical A (editor undo, `git checkout`). That +// revert is a genuine external edit and must reload — a stale self-write +// record would suppress it and leave the running config on B until restart. +func TestConfigWatcher_ExternalRevertToLastSelfWriteReloads(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + // Self-save A (records lastSelfWrite = A). + cfgA := editedConfig(initialCfg, 45678) + result, err := rt.ApplyConfig(cfgA, cfgPath) + require.NoError(t, err) + require.True(t, result.Success) + require.Equal(t, 45678, rt.ConfigSnapshot().Config.ToolResponseLimit) + + // External edit B hot-reloads. + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 11111), cfgPath)) + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 11111 + }, 5*time.Second, 25*time.Millisecond, "external edit B must hot-reload") + + // External revert back to byte-identical A must reload too. + require.NoError(t, config.SaveConfig(cfgA, cfgPath)) + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 45678 + }, 5*time.Second, 25*time.Millisecond, + "external revert to the last self-written bytes must still hot-reload") +} + // TestConfigWatcher_DebounceCoalesces: a burst of rapid writes must collapse // into one (or at most a couple of) reloads, not one per write. func TestConfigWatcher_DebounceCoalesces(t *testing.T) { From c7360d8233783bd990c924fe7dc53810233fa17e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 15 Jul 2026 22:18:46 +0300 Subject: [PATCH 04/12] fix(runtime): sync legacy r.cfg on disk reload so the API sees hot-reloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReloadConfiguration only updated the configsvc snapshot; the legacy r.cfg read by Runtime.GetConfig() — which still backs GET/PATCH /api/v1/config, profiles, tokens and docker-isolation handlers — stayed stale. After a watcher-triggered reload the API kept serving the old config, and a subsequent PATCH would deep-merge onto the stale base and save it, silently reverting the external edit on disk. Sync r.cfg/r.cfgPath from the reloaded snapshot (the legacy fallback branch already did this via UpdateConfig). Found by cross-model review. --- internal/runtime/config_watcher_test.go | 21 +++++++++++++++++++++ internal/runtime/lifecycle.go | 16 ++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index f1737dd05..2f23f1706 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -221,6 +221,27 @@ func TestConfigWatcher_ExternalRevertToLastSelfWriteReloads(t *testing.T) { "external revert to the last self-written bytes must still hot-reload") } +// TestConfigWatcher_ReloadSyncsLegacyGetConfig: a watcher reload must land in +// BOTH config surfaces — the configsvc snapshot AND the legacy r.cfg read by +// Runtime.GetConfig(), which still backs GET/PATCH /api/v1/config and other +// httpapi handlers. If only the snapshot updates, the API keeps serving the +// stale config and a subsequent PATCH would merge onto the stale base and +// save it, silently reverting the external edit on disk. +func TestConfigWatcher_ReloadSyncsLegacyGetConfig(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 77777), cfgPath)) + + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 77777 + }, 5*time.Second, 25*time.Millisecond, "snapshot must pick up the external edit") + + legacyCfg, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, 77777, legacyCfg.ToolResponseLimit, + "legacy GetConfig (backing GET/PATCH /api/v1/config) must see the reloaded config") +} + // TestConfigWatcher_DebounceCoalesces: a burst of rapid writes must collapse // into one (or at most a couple of) reloads, not one per write. func TestConfigWatcher_DebounceCoalesces(t *testing.T) { diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 35161d6e6..0559d303d 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1114,6 +1114,22 @@ func (r *Runtime) ReloadConfiguration() error { return fmt.Errorf("failed to reload config: %w", err) } + // Sync the legacy r.cfg/r.cfgPath fields too: Runtime.GetConfig() still + // backs GET/PATCH /api/v1/config and other httpapi handlers. Without this, + // a disk reload only lands in the configsvc snapshot — the API keeps + // serving the stale config, and a subsequent PATCH would deep-merge onto + // the stale base and save it, silently reverting the external edit. + // (configSvc.ReloadFromFile doesn't touch the legacy fields; the legacy + // fallback branch above already synced them via UpdateConfig.) + if r.configSvc != nil { + r.mu.Lock() + r.cfg = newSnapshot.Config + if newSnapshot.Path != "" { + r.cfgPath = newSnapshot.Path + } + r.mu.Unlock() + } + if err := r.LoadConfiguredServers(nil); err != nil { r.logger.Error("loadConfiguredServers failed", zap.Error(err)) return fmt.Errorf("failed to reload servers: %w", err) From 3733cffe40f66f05d6b56484e827ce28562113dc Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 15 Jul 2026 22:26:07 +0300 Subject: [PATCH 05/12] fix(runtime): drop stale self-write marker on snapshot-match suppression too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-2 fix cleared the recorded self-write only on the reload branch. An external revert that restores disk to the in-memory config (cancelling a pending restart-required apply) takes the snapshot-match branch and left the marker armed, so a later genuine external re-write of those self-saved bytes was suppressed until restart — its hot-reloadable parts (new servers, limits) never applied. Clear the marker whenever observed disk content diverges from it: the file has moved past our last save. Found by cross-model review. --- internal/runtime/config_watcher.go | 8 ++++++ internal/runtime/config_watcher_test.go | 36 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index 3a9d74dba..7e0030287 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -166,6 +166,14 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { trimmedDisk := bytes.TrimSpace(diskBytes) if current, merr := json.MarshalIndent(r.ConfigSnapshot().Config, "", " "); merr == nil { if bytes.Equal(bytes.TrimSpace(current), trimmedDisk) { + // The file now matches memory. If that content differs from the + // recorded self-write, the file has moved past our last save + // (e.g. an external revert cancelling a pending restart-required + // apply) — drop the stale marker so a later external re-write of + // those old self-saved bytes still reloads. + if !r.matchesLastSelfWrite(trimmedDisk) { + r.clearLastSelfWrite() + } r.logger.Debug("Config file event matches in-memory config; skipping reload", zap.String("path", absPath)) return diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index 2f23f1706..fc71f498e 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -221,6 +221,42 @@ func TestConfigWatcher_ExternalRevertToLastSelfWriteReloads(t *testing.T) { "external revert to the last self-written bytes must still hot-reload") } +// TestConfigWatcher_RevertThenRewriteOfSelfWrittenBytesReloads: the recorded +// self-write must also be invalidated when the file moves past it via the +// snapshot-match branch. Sequence: restart-required ApplyConfig saves A +// (marker=A, memory stays O), user externally reverts the file to O +// (suppressed as disk==memory), then externally writes A again. That last +// write is a genuine external edit whose hot-reloadable parts must apply — +// a stale marker would suppress it until restart. +func TestConfigWatcher_RevertThenRewriteOfSelfWrittenBytesReloads(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + limitBefore := rt.ConfigSnapshot().Config.ToolResponseLimit + + // Restart-required apply: disk=A, memory=O, marker=A. + cfgA := editedConfig(initialCfg, 88888) + cfgA.Listen = "127.0.0.1:1" + result, err := rt.ApplyConfig(cfgA, cfgPath) + require.NoError(t, err) + require.True(t, result.RequiresRestart) + + // Let the self-write event be (correctly) suppressed. + time.Sleep(1200 * time.Millisecond) + require.Equal(t, limitBefore, rt.ConfigSnapshot().Config.ToolResponseLimit) + + // External revert to O: disk==memory, suppressed — but the file has now + // moved past our last save, so the marker must be dropped here. + require.NoError(t, config.SaveConfig(initialCfg, cfgPath)) + time.Sleep(1200 * time.Millisecond) + + // External re-write of A: genuine edit, must hot-reload its hot parts. + require.NoError(t, config.SaveConfig(cfgA, cfgPath)) + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 88888 + }, 5*time.Second, 25*time.Millisecond, + "external re-write of previously self-saved bytes must hot-reload") +} + // TestConfigWatcher_ReloadSyncsLegacyGetConfig: a watcher reload must land in // BOTH config surfaces — the configsvc snapshot AND the legacy r.cfg read by // Runtime.GetConfig(), which still backs GET/PATCH /api/v1/config and other From ddfbc6ea3067c2208dbd93c17550382d0b357e36 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 15 Jul 2026 22:35:41 +0300 Subject: [PATCH 06/12] fix(runtime): propagate disk reloads to upstream global config; arm self-write marker pre-save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cross-model review findings: 1. ReloadConfiguration never called upstreamManager.SetGlobalConfig, so a watcher (or manual) disk reload updated the snapshot/API but running managed clients kept resolving the old global config — an external edit to health_check_interval or Docker-recovery settings silently never reached them, while the identical PATCH did. Propagate the reloaded config (parity with ApplyConfig, spec 074) and expose a GlobalConfig getter on the manager for the regression test. 2. ApplyConfig recorded the self-write marker only after config.SaveConfig returned; a >debounce delay between the atomic rename and the record let the watcher misread the API's own save as an external edit — for a restart-required PATCH that hot-applies a config the API just reported as deferred. Arm the marker before writing; a stale marker from a failed save is harmless (only matches byte-identical content, dropped on any diverging observation). --- internal/runtime/config_watcher_test.go | 20 ++++++++++++++++++++ internal/runtime/lifecycle.go | 9 +++++++++ internal/runtime/runtime.go | 20 +++++++++++++------- internal/upstream/manager.go | 7 +++++++ 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index fc71f498e..d21e63baf 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -278,6 +278,26 @@ func TestConfigWatcher_ReloadSyncsLegacyGetConfig(t *testing.T) { "legacy GetConfig (backing GET/PATCH /api/v1/config) must see the reloaded config") } +// TestConfigWatcher_ReloadPropagatesGlobalConfigToUpstream: a watcher reload +// must reach the upstream manager (and through it every running managed +// client), matching what ApplyConfig does via SetGlobalConfig — otherwise +// external edits to global fields like health_check_interval update the +// snapshot/API but running clients keep resolving the old values. +func TestConfigWatcher_ReloadPropagatesGlobalConfigToUpstream(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 99999), cfgPath)) + + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 99999 + }, 5*time.Second, 25*time.Millisecond, "snapshot must pick up the external edit") + + gc := rt.upstreamManager.GlobalConfig() + require.NotNil(t, gc) + assert.Equal(t, 99999, gc.ToolResponseLimit, + "watcher reload must propagate the new global config to the upstream manager") +} + // TestConfigWatcher_DebounceCoalesces: a burst of rapid writes must collapse // into one (or at most a couple of) reloads, not one per write. func TestConfigWatcher_DebounceCoalesces(t *testing.T) { diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 0559d303d..0841881c7 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1130,6 +1130,15 @@ func (r *Runtime) ReloadConfiguration() error { r.mu.Unlock() } + // Propagate the reloaded global config to the upstream manager and every + // running managed client (parity with ApplyConfig, spec 074): health-check + // loops and Docker-recovery decisions re-resolve values like + // health_check_interval from this, so external edits must reach it too — + // not only API applies. + if r.upstreamManager != nil { + r.upstreamManager.SetGlobalConfig(newSnapshot.Config) + } + if err := r.LoadConfiguredServers(nil); err != nil { r.logger.Error("loadConfiguredServers failed", zap.Error(err)) return fmt.Errorf("failed to reload servers: %w", err) diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index b85a8cea2..587d4823c 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1373,6 +1373,19 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp if savePath == "" { savePath = r.cfgPath } + + // Record the save for the config watcher's self-write suppression BEFORE + // writing, so the fs event can never outrun the marker (a >debounce pause + // between rename and record would otherwise let the watcher misread our + // own write as external). This matters most for the restart-required + // branch below: disk gets the new config while memory intentionally keeps + // the old one, so the watcher's snapshot comparison alone would misread + // our own save as an external edit and hot-apply it behind the API's back + // (config_watcher.go). If the save fails, the stale marker is harmless: + // it only ever matches disk content byte-identical to this config, and is + // dropped as soon as any diverging content is observed. + r.noteConfigSelfWrite(newCfg) + saveErr := config.SaveConfig(newCfg, savePath) if saveErr != nil { r.logger.Error("Failed to save configuration to disk", @@ -1387,13 +1400,6 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp zap.String("path", savePath)) } - // Record the save for the config watcher's self-write suppression. This - // matters most for the restart-required branch below: disk gets the new - // config while memory intentionally keeps the old one, so the watcher's - // snapshot comparison alone would misread our own save as an external - // edit and hot-apply it behind the API's back (config_watcher.go). - r.noteConfigSelfWrite(newCfg) - // If restart is required, don't apply changes in-memory (let user restart) if result.RequiresRestart { r.logger.Warn("Configuration changes require restart", diff --git a/internal/upstream/manager.go b/internal/upstream/manager.go index 3b3b6eec5..0652b7cf5 100644 --- a/internal/upstream/manager.go +++ b/internal/upstream/manager.go @@ -312,6 +312,13 @@ func (m *Manager) SetGlobalConfig(globalConfig *config.Config) { } } +// GlobalConfig returns the proxy-wide config the manager currently uses for +// newly created clients and Docker-recovery decisions (see SetGlobalConfig). +// May be nil before the first Store. +func (m *Manager) GlobalConfig() *config.Config { + return m.globalConfig.Load() +} + // AddNotificationHandler adds a notification handler to receive state change notifications func (m *Manager) AddNotificationHandler(handler NotificationHandler) { m.notificationMgr.AddHandler(handler) From 0bfbc64affc0a9558750cf4c62d4c2b7c95247c8 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 16 Jul 2026 06:49:56 +0300 Subject: [PATCH 07/12] fix(runtime,cli): clear stale self-write marker on failed save; delegate opt-out beacon to running daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ApplyConfig pre-arms the config watcher's self-write marker before config.SaveConfig; on a FAILED save the marker is now cleared again, so a later genuine external write of byte-identical JSON hot-reloads instead of being suppressed as a self-write echo. - 'mcpproxy telemetry disable' now skips its own opt-out beacon when a running daemon is detected (socket check, same pattern as the other daemon-aware commands): the daemon hot-reloads the changed config via the fsnotify watcher and fires the beacon through NotifyConfigChanged, and its once-latch is process-local — sending from both processes emitted the beacon twice. A stderr note records the delegation. --- cmd/mcpproxy/telemetry_cmd.go | 39 ++++++---- cmd/mcpproxy/telemetry_cmd_test.go | 98 +++++++++++++++++++++++++ internal/runtime/config_watcher_test.go | 30 ++++++++ internal/runtime/runtime.go | 10 ++- 4 files changed, 161 insertions(+), 16 deletions(-) diff --git a/cmd/mcpproxy/telemetry_cmd.go b/cmd/mcpproxy/telemetry_cmd.go index e3b392a74..de73cf16c 100644 --- a/cmd/mcpproxy/telemetry_cmd.go +++ b/cmd/mcpproxy/telemetry_cmd.go @@ -290,24 +290,37 @@ func runTelemetryDisable(cmd *cobra.Command, _ []string) error { // command never appears to hang on the (best-effort) beacon below. fmt.Println("Telemetry disabled.") - // One-time opt-out beacon. A running daemon now hot-reloads this file via - // the fsnotify config watcher (config_watcher.go) and fires its own beacon - // through NotifyConfigChanged — but no daemon may be running, so the CLI - // stays responsible for the beacon in the CLI-driven path. Route it - // through the SAME guarded server-side - // entry point (EmitOptOutBeacon applies the dev-build/semver, env, and - // anon-id guards and owns the single send) rather than duplicating the send - // or bypassing a guard. A short timeout keeps this from blocking on a slow - // endpoint; the CLI is short-lived so the send must complete before exit. + // One-time opt-out beacon. A running daemon hot-reloads this file via the + // fsnotify config watcher (config_watcher.go) and fires its own beacon + // through NotifyConfigChanged — its once-latch is process-local, so if the + // CLI also sent one the beacon would go out TWICE. When a daemon is + // detected (same socket check as the other daemon-aware commands), the CLI + // therefore delegates the send to it. With no daemon running the CLI stays + // responsible, routing through the SAME guarded server-side entry point + // (EmitOptOutBeacon applies the dev-build/semver, env, and anon-id guards + // and owns the single send) rather than duplicating the send or bypassing + // a guard. A short timeout keeps this from blocking on a slow endpoint; + // the CLI is short-lived so the send must complete before exit. if wasEnabled { - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) - defer cancel() - beaconSvc := telemetry.New(cfg, "", version, Edition, zap.NewNop()) - beaconSvc.EmitOptOutBeacon(ctx) + if shouldUseTelemetryDaemon(cfg.DataDir) { + fmt.Fprintln(os.Stderr, "Running mcpproxy daemon detected; opt-out beacon delegated to it (sent on config hot-reload).") + } else { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + beaconSvc := telemetry.New(cfg, "", version, Edition, zap.NewNop()) + beaconSvc.EmitOptOutBeacon(ctx) + } } return nil } +// shouldUseTelemetryDaemon checks if a daemon is running by detecting the +// socket file (same pattern as shouldUseAuthDaemon / shouldUseCallDaemon). +func shouldUseTelemetryDaemon(dataDir string) bool { + socketPath := socket.DetectSocketPath(dataDir) + return socket.IsSocketAvailable(socketPath) +} + func loadTelemetryConfig() (*config.Config, error) { if configFile != "" { cfg, err := config.LoadFromFile(configFile) diff --git a/cmd/mcpproxy/telemetry_cmd_test.go b/cmd/mcpproxy/telemetry_cmd_test.go index 7bc4f99f5..09b36f320 100644 --- a/cmd/mcpproxy/telemetry_cmd_test.go +++ b/cmd/mcpproxy/telemetry_cmd_test.go @@ -2,8 +2,11 @@ package main import ( "encoding/json" + "net/http" + "net/http/httptest" "os" "path/filepath" + "sync/atomic" "testing" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" @@ -159,3 +162,98 @@ func TestTelemetryConfigSavePath_PrefersConfigFileFlag(t *testing.T) { t.Errorf("with configFile set: got %q, want %q", got, "/elsewhere/custom.json") } } + +// writeBeaconTestConfig writes a config with telemetry enabled, a real +// anonymous_id and the opt-out beacon endpoint pointed at a test server, so +// runTelemetryDisable's beacon send (if any) is observable and hermetic. +func writeBeaconTestConfig(t *testing.T, path, dataDir, endpoint string) { + t.Helper() + cfg := map[string]interface{}{ + "listen": "127.0.0.1:0", + "data_dir": dataDir, + "telemetry": map[string]interface{}{ + "enabled": true, + "anonymous_id": "0f5b62e0-1111-4222-8333-444455556666", + "endpoint": endpoint, + }, + } + data, err := json.MarshalIndent(cfg, "", " ") + if err != nil { + t.Fatalf("marshal config: %v", err) + } + if err := os.WriteFile(path, data, 0600); err != nil { + t.Fatalf("write config: %v", err) + } +} + +// beaconTestSetup neutralizes the telemetry env kill-switches (CI is set on +// runners and would suppress the beacon entirely), sandboxes HOME, points the +// beacon endpoint at a counting test server, and wires --config. Returns the +// request counter. +func beaconTestSetup(t *testing.T) *int32 { + t.Helper() + sandboxHome(t) + t.Setenv("CI", "") + t.Setenv("DO_NOT_TRACK", "") + t.Setenv("MCPPROXY_TELEMETRY", "") + + var hits int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&hits, 1) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + tmpDir := t.TempDir() + customPath := filepath.Join(tmpDir, "custom_mcp_config.json") + writeBeaconTestConfig(t, customPath, tmpDir, srv.URL) + + prev := configFile + configFile = customPath + t.Cleanup(func() { configFile = prev }) + + return &hits +} + +// TestRunTelemetryDisable_SendsBeaconWhenNoDaemon: with no daemon socket +// present, the CLI stays responsible for the opt-out beacon and must send it +// exactly once. +func TestRunTelemetryDisable_SendsBeaconWhenNoDaemon(t *testing.T) { + hits := beaconTestSetup(t) + // Point daemon detection at a socket path that does not exist. + t.Setenv("MCPPROXY_TRAY_ENDPOINT", "unix://"+filepath.Join(t.TempDir(), "absent.sock")) + + if err := runTelemetryDisable(nil, nil); err != nil { + t.Fatalf("runTelemetryDisable: %v", err) + } + if got := atomic.LoadInt32(hits); got != 1 { + t.Errorf("beacon sends with no daemon = %d, want 1", got) + } +} + +// TestRunTelemetryDisable_DelegatesBeaconToRunningDaemon: when a daemon is +// running (socket present), it hot-reloads the changed config file via the +// fsnotify watcher and fires the beacon itself through NotifyConfigChanged — +// so the CLI must NOT also send one (the once-latch is process-local and +// cannot dedupe across the two processes). +func TestRunTelemetryDisable_DelegatesBeaconToRunningDaemon(t *testing.T) { + hits := beaconTestSetup(t) + // Simulate a running daemon: IsSocketAvailable stats the unix socket path. + sockPath := filepath.Join(t.TempDir(), "present.sock") + if err := os.WriteFile(sockPath, nil, 0600); err != nil { + t.Fatalf("create fake socket file: %v", err) + } + t.Setenv("MCPPROXY_TRAY_ENDPOINT", "unix://"+sockPath) + + if err := runTelemetryDisable(nil, nil); err != nil { + t.Fatalf("runTelemetryDisable: %v", err) + } + if got := atomic.LoadInt32(hits); got != 0 { + t.Errorf("beacon sends with daemon running = %d, want 0 (daemon owns the beacon)", got) + } + // The disable itself must still be persisted. + enabled := readTelemetryEnabled(t, configFile) + if enabled == nil || *enabled { + t.Errorf("telemetry.enabled must be persisted false even when the beacon is delegated") + } +} diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index d21e63baf..651d5561a 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -359,3 +359,33 @@ func TestConfigWatcher_MissingDirGracefulDegradation(t *testing.T) { // Runtime still works. assert.NotNil(t, rt.ConfigSnapshot()) } + +// TestConfigWatcher_FailedSelfSaveDoesNotSuppressExternalWrite: the self-write +// marker is armed BEFORE config.SaveConfig runs (pre-arming closes the +// event-outruns-marker race), so a FAILED save (permissions, disk) must clear +// it again. Otherwise a stale marker survives with bytes that never reached +// disk, and a later genuine external write of byte-identical JSON to the +// watched file would be misread as our own echo and silently suppressed. +func TestConfigWatcher_FailedSelfSaveDoesNotSuppressExternalWrite(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + // A save path whose parent is a regular file makes config.SaveConfig fail + // deterministically (MkdirAll -> ENOTDIR), independent of uid/umask. + blocker := filepath.Join(filepath.Dir(cfgPath), "blocker") + require.NoError(t, os.WriteFile(blocker, []byte("not a dir"), 0o600)) + failPath := filepath.Join(blocker, "mcp_config.json") + + cfgA := editedConfig(initialCfg, 66666) + _, err := rt.ApplyConfig(cfgA, failPath) + require.Error(t, err, "ApplyConfig must fail when the config cannot be saved") + + // Genuine external write of the SAME bytes the failed apply tried to save + // (ApplyConfig marshals the config it was handed; SaveConfig serializes it + // identically). This is a real edit — nothing ever reached disk — so it + // must hot-reload, not be suppressed by the stale pre-armed marker. + require.NoError(t, config.SaveConfig(cfgA, cfgPath)) + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 66666 + }, 5*time.Second, 25*time.Millisecond, + "external write byte-identical to a FAILED self-save must still hot-reload") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 587d4823c..b5bea94be 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1381,13 +1381,17 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp // branch below: disk gets the new config while memory intentionally keeps // the old one, so the watcher's snapshot comparison alone would misread // our own save as an external edit and hot-apply it behind the API's back - // (config_watcher.go). If the save fails, the stale marker is harmless: - // it only ever matches disk content byte-identical to this config, and is - // dropped as soon as any diverging content is observed. + // (config_watcher.go). If the save fails, the marker is cleared again on + // the error path below — nothing reached disk, so a later byte-identical + // EXTERNAL write of this config is a genuine edit the watcher must reload. r.noteConfigSelfWrite(newCfg) saveErr := config.SaveConfig(newCfg, savePath) if saveErr != nil { + // Drop the pre-armed self-write marker: the save never landed, so no + // future fs event for these bytes can be our own echo. Keeping it + // would suppress a genuine external write of byte-identical JSON. + r.clearLastSelfWrite() r.logger.Error("Failed to save configuration to disk", zap.String("path", savePath), zap.Error(saveErr)) From 2f628ecd49f93d919812c10482594799e12e73eb Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 16 Jul 2026 07:13:59 +0300 Subject: [PATCH 08/12] fix(runtime): share live component side effects between ApplyConfig and disk reload; revert beacon delegation - Extract applyComponentConfigLocked (logging via SetLogConfig, tool-response truncator rebuild, observability usage cadence) and call it from BOTH ApplyConfig and ReloadConfiguration, so watcher/disk reloads apply the same live side effects as API applies instead of only updating the snapshot. The helper compares fields directly (mirroring DetectConfigChanges) because DetectConfigChanges returns early on restart-required fields and would under-report hot changes riding along in the same external edit. - Revert the telemetry opt-out beacon delegation: the CLI always sends after persisting the disable. Accepted trade-off documented at the send site: a running daemon may double-send via hot-reload (benign; backend dedupes by anon_id), whereas a stat-only socket false positive would drop the only send. Removes shouldUseTelemetryDaemon and its delegation test; keeps an always-sends-once regression test. --- cmd/mcpproxy/telemetry_cmd.go | 40 ++++++-------- cmd/mcpproxy/telemetry_cmd_test.go | 40 ++++---------- internal/runtime/config_watcher_test.go | 30 +++++++++++ internal/runtime/lifecycle.go | 9 ++++ internal/runtime/runtime.go | 72 ++++++++++++++++--------- 5 files changed, 112 insertions(+), 79 deletions(-) diff --git a/cmd/mcpproxy/telemetry_cmd.go b/cmd/mcpproxy/telemetry_cmd.go index de73cf16c..25771afd1 100644 --- a/cmd/mcpproxy/telemetry_cmd.go +++ b/cmd/mcpproxy/telemetry_cmd.go @@ -290,37 +290,29 @@ func runTelemetryDisable(cmd *cobra.Command, _ []string) error { // command never appears to hang on the (best-effort) beacon below. fmt.Println("Telemetry disabled.") - // One-time opt-out beacon. A running daemon hot-reloads this file via the - // fsnotify config watcher (config_watcher.go) and fires its own beacon - // through NotifyConfigChanged — its once-latch is process-local, so if the - // CLI also sent one the beacon would go out TWICE. When a daemon is - // detected (same socket check as the other daemon-aware commands), the CLI - // therefore delegates the send to it. With no daemon running the CLI stays - // responsible, routing through the SAME guarded server-side entry point + // One-time opt-out beacon. The CLI ALWAYS sends it after persisting the + // disable, routing through the SAME guarded server-side entry point // (EmitOptOutBeacon applies the dev-build/semver, env, and anon-id guards // and owns the single send) rather than duplicating the send or bypassing - // a guard. A short timeout keeps this from blocking on a slow endpoint; - // the CLI is short-lived so the send must complete before exit. + // a guard. Accepted trade-off (PR #857): a running daemon hot-reloads this + // file via the fsnotify config watcher and may ALSO emit the beacon from + // its NotifyConfigChanged path — that double-send is benign (the telemetry + // backend dedupes opt-outs by anon_id), whereas gating the CLI send on + // daemon detection risks DROPPING the beacon entirely on a false positive + // (the socket check only stat()s the path, so a stale socket file — or a + // daemon on the same data dir that doesn't watch this --config file — + // would suppress the only send). A short timeout keeps this from blocking + // on a slow endpoint; the CLI is short-lived so the send must complete + // before exit. if wasEnabled { - if shouldUseTelemetryDaemon(cfg.DataDir) { - fmt.Fprintln(os.Stderr, "Running mcpproxy daemon detected; opt-out beacon delegated to it (sent on config hot-reload).") - } else { - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) - defer cancel() - beaconSvc := telemetry.New(cfg, "", version, Edition, zap.NewNop()) - beaconSvc.EmitOptOutBeacon(ctx) - } + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + beaconSvc := telemetry.New(cfg, "", version, Edition, zap.NewNop()) + beaconSvc.EmitOptOutBeacon(ctx) } return nil } -// shouldUseTelemetryDaemon checks if a daemon is running by detecting the -// socket file (same pattern as shouldUseAuthDaemon / shouldUseCallDaemon). -func shouldUseTelemetryDaemon(dataDir string) bool { - socketPath := socket.DetectSocketPath(dataDir) - return socket.IsSocketAvailable(socketPath) -} - func loadTelemetryConfig() (*config.Config, error) { if configFile != "" { cfg, err := config.LoadFromFile(configFile) diff --git a/cmd/mcpproxy/telemetry_cmd_test.go b/cmd/mcpproxy/telemetry_cmd_test.go index 09b36f320..409183d6d 100644 --- a/cmd/mcpproxy/telemetry_cmd_test.go +++ b/cmd/mcpproxy/telemetry_cmd_test.go @@ -215,45 +215,23 @@ func beaconTestSetup(t *testing.T) *int32 { return &hits } -// TestRunTelemetryDisable_SendsBeaconWhenNoDaemon: with no daemon socket -// present, the CLI stays responsible for the opt-out beacon and must send it -// exactly once. -func TestRunTelemetryDisable_SendsBeaconWhenNoDaemon(t *testing.T) { +// TestRunTelemetryDisable_SendsBeacon: the CLI always sends the opt-out +// beacon (exactly once) after persisting the disable — it does NOT gate the +// send on daemon detection. Accepted trade-off (PR #857): a running daemon +// may also emit the beacon on config hot-reload, and the backend dedupes by +// anon_id; a detection false positive dropping the only send would be worse. +func TestRunTelemetryDisable_SendsBeacon(t *testing.T) { hits := beaconTestSetup(t) - // Point daemon detection at a socket path that does not exist. - t.Setenv("MCPPROXY_TRAY_ENDPOINT", "unix://"+filepath.Join(t.TempDir(), "absent.sock")) if err := runTelemetryDisable(nil, nil); err != nil { t.Fatalf("runTelemetryDisable: %v", err) } if got := atomic.LoadInt32(hits); got != 1 { - t.Errorf("beacon sends with no daemon = %d, want 1", got) + t.Errorf("beacon sends = %d, want 1", got) } -} - -// TestRunTelemetryDisable_DelegatesBeaconToRunningDaemon: when a daemon is -// running (socket present), it hot-reloads the changed config file via the -// fsnotify watcher and fires the beacon itself through NotifyConfigChanged — -// so the CLI must NOT also send one (the once-latch is process-local and -// cannot dedupe across the two processes). -func TestRunTelemetryDisable_DelegatesBeaconToRunningDaemon(t *testing.T) { - hits := beaconTestSetup(t) - // Simulate a running daemon: IsSocketAvailable stats the unix socket path. - sockPath := filepath.Join(t.TempDir(), "present.sock") - if err := os.WriteFile(sockPath, nil, 0600); err != nil { - t.Fatalf("create fake socket file: %v", err) - } - t.Setenv("MCPPROXY_TRAY_ENDPOINT", "unix://"+sockPath) - - if err := runTelemetryDisable(nil, nil); err != nil { - t.Fatalf("runTelemetryDisable: %v", err) - } - if got := atomic.LoadInt32(hits); got != 0 { - t.Errorf("beacon sends with daemon running = %d, want 0 (daemon owns the beacon)", got) - } - // The disable itself must still be persisted. + // The disable itself must be persisted. enabled := readTelemetryEnabled(t, configFile) if enabled == nil || *enabled { - t.Errorf("telemetry.enabled must be persisted false even when the beacon is delegated") + t.Errorf("telemetry.enabled must be persisted false") } } diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index 651d5561a..d339774c9 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" "time" @@ -389,3 +390,32 @@ func TestConfigWatcher_FailedSelfSaveDoesNotSuppressExternalWrite(t *testing.T) }, 5*time.Second, 25*time.Millisecond, "external write byte-identical to a FAILED self-save must still hot-reload") } + +// TestConfigWatcher_ReloadRebuildsTruncator: a watcher disk reload must apply +// the same live component side effects as ApplyConfig (PR #857 round-6 review) +// — here the tool-response truncator: an external edit to tool_response_limit +// must change actual truncation behavior, not just the snapshot value. +func TestConfigWatcher_ReloadRebuildsTruncator(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + content := strings.Repeat("x", 200) + pre := rt.Truncator().Truncate(content, "srv:tool", nil) + require.Equal(t, content, pre.TruncatedContent, + "sanity: the default limit (20000) must not truncate 200 chars") + + // External edit shrinks the limit below the content size. + require.NoError(t, config.SaveConfig(editedConfig(initialCfg, 50), cfgPath)) + require.Eventually(t, func() bool { + return rt.ConfigSnapshot().Config.ToolResponseLimit == 50 + }, 5*time.Second, 25*time.Millisecond, "snapshot must pick up the external edit") + + // The LIVE truncator must now enforce the new limit. Read it under r.mu: + // the reload path swaps the field under the same lock. + require.Eventually(t, func() bool { + rt.mu.RLock() + tr := rt.truncator + rt.mu.RUnlock() + return len(tr.Truncate(content, "srv:tool", nil).TruncatedContent) < len(content) + }, 5*time.Second, 25*time.Millisecond, + "watcher reload must rebuild the truncator with the new tool_response_limit") +} diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 0841881c7..d348d31b7 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1139,6 +1139,15 @@ func (r *Runtime) ReloadConfiguration() error { r.upstreamManager.SetGlobalConfig(newSnapshot.Config) } + // Parity with ApplyConfig's live per-component side effects (PR #857 + // review): logging via SetLogConfig, the tool-response truncator, and the + // observability usage cadence must follow disk reloads too — otherwise an + // external edit lands in the snapshot/API while the running components + // keep their stale values. + r.mu.Lock() + r.applyComponentConfigLocked(oldSnapshot.Config, newSnapshot.Config) + r.mu.Unlock() + if err := r.LoadConfiguredServers(nil); err != nil { r.logger.Error("loadConfiguredServers failed", zap.Error(err)) return fmt.Errorf("failed to reload servers: %w", err) diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index b5bea94be..5e2466c37 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -10,6 +10,7 @@ import ( "net" "os" "os/exec" + "reflect" "sort" "strings" "sync" @@ -1433,30 +1434,10 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp r.logger.Info("Applying configuration hot-reload", zap.Strings("changed_fields", result.ChangedFields)) - // Update logging configuration - if contains(result.ChangedFields, "logging") { - r.logger.Info("Logging configuration changed") - if r.upstreamManager != nil && newCfg.Logging != nil { - r.upstreamManager.SetLogConfig(newCfg.Logging) - } - } - - // Update truncator if tool response limit changed - if contains(result.ChangedFields, "tool_response_limit") { - r.logger.Info("Tool response limit changed, updating truncator", - zap.Int("old_limit", oldCfg.ToolResponseLimit), - zap.Int("new_limit", newCfg.ToolResponseLimit)) - r.truncator = truncate.NewTruncator(newCfg.ToolResponseLimit) - } - - // Apply observability usage cadence (Spec 069 A2 — hot-reloadable). The - // usage flush loop re-reads the interval each cycle, so the setter suffices. - if contains(result.ChangedFields, "observability") && r.activityService != nil && - newCfg.Observability != nil && newCfg.Observability.UsagePersistInterval.Duration() > 0 { - r.logger.Info("Observability usage persist interval changed", - zap.Duration("new_interval", newCfg.Observability.UsagePersistInterval.Duration())) - r.activityService.SetUsagePersistInterval(newCfg.Observability.UsagePersistInterval.Duration()) - } + // Apply live per-component side effects (logging, truncator, observability + // cadence) — shared with the disk-reload path (ReloadConfiguration) so the + // two config paths cannot drift. + r.applyComponentConfigLocked(oldCfg, newCfg) // Apply update-check settings (Spec 079 FR-012 — hot-reloadable). The // checker gates its poll + CheckNow on the flag internally; a @@ -2523,6 +2504,49 @@ func (r *Runtime) applyUpdateCheckConfig(cfg *config.Config) { r.updateChecker.SetConfig(uc.IsEnabled(), uc.IncludePrereleases()) } +// applyComponentConfigLocked applies the live per-component side effects of a +// hot-reloaded configuration: upstream log config, the tool-response +// truncator, and the observability usage-persist cadence. It is shared by +// ApplyConfig (API path) and ReloadConfiguration (disk/watcher path) so the +// two paths cannot drift — the PR #857 review found disk reloads updating the +// snapshot while these live components kept running on stale values. +// +// It performs its own field comparisons (mirroring DetectConfigChanges) +// instead of taking a ChangedFields list, because DetectConfigChanges returns +// early on restart-required fields and would under-report hot-reloadable +// changes riding along in the same external edit. Caller must hold r.mu. +func (r *Runtime) applyComponentConfigLocked(oldCfg, newCfg *config.Config) { + if oldCfg == nil || newCfg == nil { + return + } + + // Update logging configuration + if newCfg.Logging != nil && !reflect.DeepEqual(oldCfg.Logging, newCfg.Logging) { + r.logger.Info("Logging configuration changed") + if r.upstreamManager != nil { + r.upstreamManager.SetLogConfig(newCfg.Logging) + } + } + + // Update truncator if tool response limit changed + if oldCfg.ToolResponseLimit != newCfg.ToolResponseLimit { + r.logger.Info("Tool response limit changed, updating truncator", + zap.Int("old_limit", oldCfg.ToolResponseLimit), + zap.Int("new_limit", newCfg.ToolResponseLimit)) + r.truncator = truncate.NewTruncator(newCfg.ToolResponseLimit) + } + + // Apply observability usage cadence (Spec 069 A2 — hot-reloadable). The + // usage flush loop re-reads the interval each cycle, so the setter suffices. + if r.activityService != nil && newCfg.Observability != nil && + newCfg.Observability.UsagePersistInterval.Duration() > 0 && + !reflect.DeepEqual(oldCfg.Observability, newCfg.Observability) { + r.logger.Info("Observability usage persist interval changed", + zap.Duration("new_interval", newCfg.Observability.UsagePersistInterval.Duration())) + r.activityService.SetUsagePersistInterval(newCfg.Observability.UsagePersistInterval.Duration()) + } +} + // GetVersionInfo returns the current version information from the update checker. // Returns nil if the update checker has not been initialized. func (r *Runtime) GetVersionInfo() *updatecheck.VersionInfo { From c9a369ab96f44c71a5c5f577d0d01d8dda60de15 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 16 Jul 2026 07:32:43 +0300 Subject: [PATCH 09/12] fix(runtime): replace single-slot config self-write marker with bounded recent-writes set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Back-to-back ApplyConfig saves could evict each other's self-write marker before the watcher debounce fired: pre-arming save B overwrote the record of restart-required save A, so the watcher read disk=A, failed the marker check, and hot-applied A behind the API's back (violating the requires_restart contract) — and clearing on that 'external' reload then let B be hot-applied too. The marker is now a small set (last 4 payloads, 10s TTL — generous vs the ~500ms echo window) keyed by marshaled content. All established semantics preserved: pre-arm before write, per-payload forget on save error, keep-on-match within TTL, clear-all on genuine external change and on divergence past our saves. --- internal/runtime/config_watcher.go | 126 +++++++++++++++++++----- internal/runtime/config_watcher_test.go | 38 +++++++ internal/runtime/runtime.go | 24 +++-- 3 files changed, 151 insertions(+), 37 deletions(-) diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index 7e0030287..f588d39e5 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -104,44 +104,116 @@ func (r *Runtime) runConfigWatchLoop(ctx context.Context, watcher *fsnotify.Watc } } +// Self-write suppression bounds. Entries expire after selfWriteTTL: the +// suppression only needs to cover the echo window of our own write (the +// ~500ms debounce plus fs latency), and any event long after that is an +// external edit — 10s is generous. The set keeps at most selfWriteMaxEntries +// payloads so back-to-back ApplyConfig saves cannot evict each other's +// still-pending markers (single-slot race, PR #857 round 7). +const ( + selfWriteTTL = 10 * time.Second + selfWriteMaxEntries = 4 +) + +// selfWriteEntry is one recorded self-written config payload (trimmed +// marshaled bytes) with the time it was armed, for TTL expiry. +type selfWriteEntry struct { + payload []byte + at time.Time +} + // noteConfigSelfWrite records the marshaled form of a config mcpproxy itself -// just saved to disk, so the watcher can suppress the echo even when the -// in-memory snapshot intentionally diverges from the file — the +// is about to save to disk, so the watcher can suppress the echo even when +// the in-memory snapshot intentionally diverges from the file — the // restart-required ApplyConfig path saves to disk but defers the in-memory // apply until restart (`requires_restart` contract), and the watcher must not // hot-apply it behind the API's back. Marshals exactly as config.SaveConfig // does; a marshal failure just skips recording (the write itself would have -// failed the same way). +// failed the same way). Recording an already-present payload refreshes its +// timestamp; when full, the oldest entry is evicted. func (r *Runtime) noteConfigSelfWrite(cfg *config.Config) { data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return } + payload := bytes.TrimSpace(data) + now := time.Now() + r.selfWriteMu.Lock() - r.lastSelfWrite = bytes.TrimSpace(data) - r.selfWriteMu.Unlock() + defer r.selfWriteMu.Unlock() + r.pruneExpiredSelfWritesLocked(now) + for i := range r.recentSelfWrites { + if bytes.Equal(r.recentSelfWrites[i].payload, payload) { + r.recentSelfWrites[i].at = now + return + } + } + r.recentSelfWrites = append(r.recentSelfWrites, selfWriteEntry{payload: payload, at: now}) + if len(r.recentSelfWrites) > selfWriteMaxEntries { + r.recentSelfWrites = r.recentSelfWrites[len(r.recentSelfWrites)-selfWriteMaxEntries:] + } +} + +// forgetConfigSelfWrite removes the entry for a config whose save FAILED: +// those bytes never reached disk, so a later byte-identical write of them is +// a genuine external edit the watcher must reload. Only the failed payload is +// removed — markers pre-armed by other (successful) saves stay live. +func (r *Runtime) forgetConfigSelfWrite(cfg *config.Config) { + data, err := json.MarshalIndent(cfg, "", " ") + if err != nil { + return + } + payload := bytes.TrimSpace(data) + + r.selfWriteMu.Lock() + defer r.selfWriteMu.Unlock() + kept := r.recentSelfWrites[:0] + for _, e := range r.recentSelfWrites { + if !bytes.Equal(e.payload, payload) { + kept = append(kept, e) + } + } + r.recentSelfWrites = kept } -// matchesLastSelfWrite reports whether trimmed disk bytes equal the last -// config mcpproxy itself saved (see noteConfigSelfWrite). -func (r *Runtime) matchesLastSelfWrite(trimmedDisk []byte) bool { +// matchesRecentSelfWrite reports whether trimmed disk bytes equal any config +// mcpproxy itself recently saved (see noteConfigSelfWrite). A match does NOT +// remove the entry: within the TTL, repeat events for the same bytes (e.g. +// the pending restart-required state) must keep suppressing. +func (r *Runtime) matchesRecentSelfWrite(trimmedDisk []byte) bool { r.selfWriteMu.Lock() defer r.selfWriteMu.Unlock() - return len(r.lastSelfWrite) > 0 && bytes.Equal(r.lastSelfWrite, trimmedDisk) + r.pruneExpiredSelfWritesLocked(time.Now()) + for _, e := range r.recentSelfWrites { + if bytes.Equal(e.payload, trimmedDisk) { + return true + } + } + return false } -// clearLastSelfWrite drops the recorded self-write. Called when a genuine +// clearSelfWrites drops every recorded self-write. Called when a genuine // external change is about to reload: from that point the file's history has -// diverged from our last save, and keeping the stale record would suppress a +// diverged from our saves, and keeping the stale records would suppress a // later external revert to those exact bytes (editor undo, `git checkout`). -// It is NOT cleared on a suppression match itself, so the restart-required -// pending state keeps suppressing repeat events for the same bytes. -func (r *Runtime) clearLastSelfWrite() { +func (r *Runtime) clearSelfWrites() { r.selfWriteMu.Lock() - r.lastSelfWrite = nil + r.recentSelfWrites = nil r.selfWriteMu.Unlock() } +// pruneExpiredSelfWritesLocked drops entries older than selfWriteTTL. Caller +// must hold selfWriteMu. +func (r *Runtime) pruneExpiredSelfWritesLocked(now time.Time) { + kept := r.recentSelfWrites[:0] + for _, e := range r.recentSelfWrites { + if now.Sub(e.at) <= selfWriteTTL { + kept = append(kept, e) + } + } + r.recentSelfWrites = kept +} + // reloadFromDiskIfChanged reloads the configuration from disk unless the file // content matches the in-memory snapshot or the last self-written config // (self-write suppression: our own ApplyConfig / SaveConfiguration writes @@ -166,13 +238,13 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { trimmedDisk := bytes.TrimSpace(diskBytes) if current, merr := json.MarshalIndent(r.ConfigSnapshot().Config, "", " "); merr == nil { if bytes.Equal(bytes.TrimSpace(current), trimmedDisk) { - // The file now matches memory. If that content differs from the - // recorded self-write, the file has moved past our last save + // The file now matches memory. If that content matches none of + // the recorded self-writes, the file has moved past our saves // (e.g. an external revert cancelling a pending restart-required - // apply) — drop the stale marker so a later external re-write of + // apply) — drop the stale markers so a later external re-write of // those old self-saved bytes still reloads. - if !r.matchesLastSelfWrite(trimmedDisk) { - r.clearLastSelfWrite() + if !r.matchesRecentSelfWrite(trimmedDisk) { + r.clearSelfWrites() } r.logger.Debug("Config file event matches in-memory config; skipping reload", zap.String("path", absPath)) @@ -182,17 +254,17 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { // Restart-required applies save to disk without touching memory, so the // snapshot comparison above can't recognize them as ours — check the - // recorded last self-write too. - if r.matchesLastSelfWrite(trimmedDisk) { - r.logger.Debug("Config file event matches mcpproxy's own last save; skipping reload", + // recorded recent self-writes too. + if r.matchesRecentSelfWrite(trimmedDisk) { + r.logger.Debug("Config file event matches one of mcpproxy's own recent saves; skipping reload", zap.String("path", absPath)) return } - // Genuine external change: the file no longer descends from our last - // save, so the recorded self-write is stale — drop it (see - // clearLastSelfWrite for the revert scenario it would otherwise break). - r.clearLastSelfWrite() + // Genuine external change: the file no longer descends from our saves, + // so the recorded self-writes are stale — drop them (see clearSelfWrites + // for the revert scenario they would otherwise break). + r.clearSelfWrites() r.logger.Info("Config file changed on disk, hot-reloading", zap.String("path", absPath)) if err := r.ReloadConfiguration(); err != nil { diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index d339774c9..2261cf335 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -419,3 +419,41 @@ func TestConfigWatcher_ReloadRebuildsTruncator(t *testing.T) { }, 5*time.Second, 25*time.Millisecond, "watcher reload must rebuild the truncator with the new tool_response_limit") } + +// TestConfigWatcher_BackToBackSelfWritesBothSuppressed reproduces the PR #857 +// round-7 marker race: with a SINGLE self-write slot, a second ApplyConfig +// pre-arming its marker before the first save's debounce fires overwrites the +// first record — the watcher then reads disk=A, fails the marker check, and +// hot-applies a restart-required config behind the API's back. A bounded +// recent-self-writes set must suppress BOTH payloads. +func TestConfigWatcher_BackToBackSelfWritesBothSuppressed(t *testing.T) { + rt, initialCfg, cfgPath := newWatcherTestRuntime(t) + + limitBefore := rt.ConfigSnapshot().Config.ToolResponseLimit + + // Restart-required self-save A: disk=A, memory stays O, marker records A. + cfgA := editedConfig(initialCfg, 71111) + cfgA.Listen = "127.0.0.1:1" + resA, err := rt.ApplyConfig(cfgA, cfgPath) + require.NoError(t, err) + require.True(t, resA.RequiresRestart) + + // Second self-save B pre-arms its marker IMMEDIATELY — before A's debounce + // fires — with its own disk write still in flight (ApplyConfig pre-arms + // before config.SaveConfig; a slow marshal/fsync/rename widens exactly + // this window). With a single slot this evicts A's record. + cfgB := editedConfig(initialCfg, 72222) + cfgB.Listen = "127.0.0.1:2" + rt.noteConfigSelfWrite(cfgB) + + // A's debounce fires while disk still holds A. It must stay suppressed. + time.Sleep(1200 * time.Millisecond) + require.Equal(t, limitBefore, rt.ConfigSnapshot().Config.ToolResponseLimit, + "watcher must not hot-apply restart-required save A when a second self-write pre-armed in between") + + // B's write lands on disk; that event must be suppressed too. + require.NoError(t, config.SaveConfig(cfgB, cfgPath)) + time.Sleep(1200 * time.Millisecond) + require.Equal(t, limitBefore, rt.ConfigSnapshot().Config.ToolResponseLimit, + "watcher must not hot-apply restart-required save B either") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 5e2466c37..8cb5b6eaa 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -65,13 +65,15 @@ type Runtime struct { running bool // Config-watcher self-write suppression (config_watcher.go): marshaled - // bytes of the last config mcpproxy itself saved to disk. Needed on top - // of the snapshot comparison because a restart-required ApplyConfig saves - // the new config to disk while intentionally leaving memory on the old - // one (requires_restart contract) — the snapshot alone can't identify - // that write as ours. - selfWriteMu sync.Mutex - lastSelfWrite []byte + // bytes of the configs mcpproxy itself recently saved to disk. Needed on + // top of the snapshot comparison because a restart-required ApplyConfig + // saves the new config to disk while intentionally leaving memory on the + // old one (requires_restart contract) — the snapshot alone can't identify + // that write as ours. A bounded set rather than a single slot: back-to-back + // ApplyConfig saves must not evict each other's still-pending markers + // before the watcher debounce fires (PR #857 round-7 race). + selfWriteMu sync.Mutex + recentSelfWrites []selfWriteEntry statusMu sync.RWMutex status Status @@ -1382,17 +1384,19 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp // branch below: disk gets the new config while memory intentionally keeps // the old one, so the watcher's snapshot comparison alone would misread // our own save as an external edit and hot-apply it behind the API's back - // (config_watcher.go). If the save fails, the marker is cleared again on + // (config_watcher.go). If the save fails, its entry is removed again on // the error path below — nothing reached disk, so a later byte-identical // EXTERNAL write of this config is a genuine edit the watcher must reload. r.noteConfigSelfWrite(newCfg) saveErr := config.SaveConfig(newCfg, savePath) if saveErr != nil { - // Drop the pre-armed self-write marker: the save never landed, so no + // Drop the pre-armed self-write entry: the save never landed, so no // future fs event for these bytes can be our own echo. Keeping it // would suppress a genuine external write of byte-identical JSON. - r.clearLastSelfWrite() + // Only this payload is forgotten — markers from other still-pending + // successful saves stay live. + r.forgetConfigSelfWrite(newCfg) r.logger.Error("Failed to save configuration to disk", zap.String("path", savePath), zap.Error(saveErr)) From e577f6bed2a5da8c251160ed0bbfaf2ad52fb49d Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 22 Jul 2026 08:14:07 +0300 Subject: [PATCH 10/12] fix(runtime): serialize config commits so ApplyConfig and disk reload can't diverge ReloadConfiguration updated configSvc (via ReloadFromFile) before taking r.mu to set r.cfg, while ApplyConfig set r.cfg under r.mu and only updated configSvc after releasing it. A watcher-triggered reload of config A interleaving with an API apply of B could end with r.cfg=A while configSvc=B permanently, so the REST/snapshot surfaces disagreed with the live components. The new fsnotify config watcher makes this newly reachable. Add a dedicated configCommitMu held across the whole load-validate-commit sequence in both paths so they can never interleave. It is always acquired outside r.mu (consistent ordering, no deadlock); configSvc updates only notify subscribers over buffered channels, so no callback re-enters the runtime. Regression test runs both paths concurrently and asserts r.cfg and configSvc converge. --- internal/runtime/config_commit_race_test.go | 82 +++++++++++++++++++++ internal/runtime/lifecycle.go | 10 +++ internal/runtime/runtime.go | 22 ++++++ 3 files changed, 114 insertions(+) create mode 100644 internal/runtime/config_commit_race_test.go diff --git a/internal/runtime/config_commit_race_test.go b/internal/runtime/config_commit_race_test.go new file mode 100644 index 000000000..19e75a12c --- /dev/null +++ b/internal/runtime/config_commit_race_test.go @@ -0,0 +1,82 @@ +package runtime + +import ( + "path/filepath" + "sync" + "testing" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + + "github.com/stretchr/testify/require" +) + +// TestConfigCommit_ApplyAndReloadConverge is a regression test for the PR #857 +// config-store divergence race. +// +// The runtime keeps the active config in TWO stores that must stay in sync: +// - configSvc (served by ConfigSnapshot / Config() / live subscribers), and +// - the legacy r.cfg field (served by GetConfig / the REST /api/v1/config +// handlers). +// +// ReloadConfiguration (disk-reload / fsnotify-watcher path) updates configSvc +// via ReloadFromFile BEFORE taking r.mu to set r.cfg, while ApplyConfig (API +// path) sets r.cfg under r.mu and only updates configSvc AFTER releasing it. +// A watcher-triggered reload of config A interleaving with an API apply of B +// could therefore end with r.cfg=A while configSvc=B permanently — the REST +// surface and the live components disagreeing with no further event to +// reconcile them. +// +// Running the two paths concurrently many times, the two stores must always +// converge to the same value after each round. Before the fix (a shared +// configCommitMu serializing the whole commit in both paths) this fails on +// some round; after it, it always converges. This is a logical lost-update +// race, not a data race, so -race alone won't flag it — the convergence +// assertion is what catches it. +func TestConfigCommit_ApplyAndReloadConverge(t *testing.T) { + tmpDir := t.TempDir() + cfgPath := filepath.Join(tmpDir, "mcp_config.json") + + base := config.DefaultConfig() + base.Listen = "127.0.0.1:0" + base.DataDir = tmpDir + require.NoError(t, config.SaveConfig(base, cfgPath)) + + rt, err := New(base, cfgPath, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + + const rounds = 200 + for i := 0; i < rounds; i++ { + // Disk currently holds the previous round's config; the reload reads + // whatever is on disk, while the apply writes a fresh, distinct value + // (and overwrites disk). Distinct limits make a divergence observable. + applyLimit := 100000 + i + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + _ = rt.ReloadConfiguration() + }() + go func(limit int) { + defer wg.Done() + edited := config.DefaultConfig() + edited.Listen = base.Listen + edited.DataDir = base.DataDir + edited.ToolResponseLimit = limit + _, _ = rt.ApplyConfig(edited, cfgPath) + }(applyLimit) + wg.Wait() + + // r.cfg (GetConfig / REST) and configSvc (ConfigSnapshot / subscribers) + // must report the same config once both commits have settled. + legacy, gerr := rt.GetConfig() + require.NoError(t, gerr) + snap := rt.ConfigSnapshot().Config + require.Equal(t, legacy.ToolResponseLimit, snap.ToolResponseLimit, + "round %d: r.cfg and configSvc diverged (r.cfg=%d configSvc=%d)", + i, legacy.ToolResponseLimit, snap.ToolResponseLimit) + } +} diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index d348d31b7..bae01025b 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1088,6 +1088,16 @@ func (r *Runtime) SaveConfiguration() error { func (r *Runtime) ReloadConfiguration() error { r.logger.Info("Reloading configuration from disk") + // Serialize the whole reload against ApplyConfig: both paths update the + // configSvc snapshot AND the legacy r.cfg/live components, but in different + // orders and releasing r.mu in between. Without this outer lock a + // watcher-triggered reload of config A can interleave with an API apply of + // B and leave r.cfg=A while configSvc=B persistently (PR #857 review). Held + // across configSvc.ReloadFromFile → r.cfg swap → live-component apply. + // MUST be acquired before r.mu (matches ApplyConfig's lock ordering). + r.configCommitMu.Lock() + defer r.configCommitMu.Unlock() + // Get current snapshot before reload oldSnapshot := r.ConfigSnapshot() oldServerCount := oldSnapshot.ServerCount() diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 8cb5b6eaa..6c59c9b30 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -64,6 +64,21 @@ type Runtime struct { mu sync.RWMutex running bool + // configCommitMu serializes whole config-commit operations against each + // other. ApplyConfig (API path) and ReloadConfiguration (disk-reload path, + // incl. the fsnotify watcher) each update TWO stores that must stay in + // sync — configSvc (snapshot/subscribers) and the legacy r.cfg/live + // components — but they do so under r.mu in different orders and release + // r.mu between the two stores. Without a wider lock, a watcher reload of + // config A can interleave with an API apply of B and leave r.cfg=A while + // configSvc=B persistently (PR #857 review). This mutex is held across the + // entire load→validate→commit sequence in both paths so they can never + // interleave. Lock ordering: acquire configCommitMu OUTSIDE r.mu — never + // take configCommitMu while holding r.mu. configSvc updates only notify + // subscribers over buffered channels (no synchronous re-entry into the + // runtime), so there is no callback deadlock. + configCommitMu sync.Mutex + // Config-watcher self-write suppression (config_watcher.go): marshaled // bytes of the configs mcpproxy itself recently saved to disk. Needed on // top of the snapshot comparison because a restart-required ApplyConfig @@ -1344,6 +1359,13 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp }, fmt.Errorf("config cannot be nil") } + // Serialize the whole commit against ReloadConfiguration so the two config + // stores (configSvc + r.cfg/live components) can't be updated in an + // interleaved order that leaves them divergent. Held across save → + // r.cfg swap → configSvc.Update below. MUST be acquired before r.mu. + r.configCommitMu.Lock() + defer r.configCommitMu.Unlock() + r.mu.Lock() // Validate the new configuration first From 9f5479099b0c6271380df7fffaac7ef98e081cb0 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 22 Jul 2026 08:23:17 +0300 Subject: [PATCH 11/12] fix(runtime): serialize UpdateConfig and SaveConfiguration under configCommitMu too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex re-review of PR #857 flagged that UpdateConfig (registry-source CRUD paths) and SaveConfiguration (enable/quarantine/bulk paths) also commit to both config stores (configSvc + legacy r.cfg) without configCommitMu, so a concurrent ApplyConfig/ReloadConfiguration could still leave the two stores divergent — or SaveConfiguration's snapshot-read-then-write could clobber a config another path just applied. Extend the serialization to every two-store commit path: - UpdateConfig now takes configCommitMu; its body moves to updateConfigLocked so ReloadConfiguration's legacy configSvc==nil fallback can reuse it without re-acquiring the non-reentrant mutex. - SaveConfiguration takes configCommitMu across its whole snapshot-read -> configSvc.Update -> r.cfg.Servers write. All acquire configCommitMu outside r.mu (consistent ordering, no deadlock). Broaden the regression test to drive ApplyConfig, ReloadConfiguration, UpdateConfig and SaveConfiguration concurrently and assert convergence. --- internal/runtime/config_commit_race_test.go | 70 ++++++++++++--------- internal/runtime/lifecycle.go | 15 ++++- internal/runtime/runtime.go | 13 ++++ 3 files changed, 69 insertions(+), 29 deletions(-) diff --git a/internal/runtime/config_commit_race_test.go b/internal/runtime/config_commit_race_test.go index 19e75a12c..3ff54cc80 100644 --- a/internal/runtime/config_commit_race_test.go +++ b/internal/runtime/config_commit_race_test.go @@ -12,7 +12,7 @@ import ( "github.com/stretchr/testify/require" ) -// TestConfigCommit_ApplyAndReloadConverge is a regression test for the PR #857 +// TestConfigCommit_CommitPathsConverge is a regression test for the PR #857 // config-store divergence race. // // The runtime keeps the active config in TWO stores that must stay in sync: @@ -20,21 +20,23 @@ import ( // - the legacy r.cfg field (served by GetConfig / the REST /api/v1/config // handlers). // -// ReloadConfiguration (disk-reload / fsnotify-watcher path) updates configSvc -// via ReloadFromFile BEFORE taking r.mu to set r.cfg, while ApplyConfig (API -// path) sets r.cfg under r.mu and only updates configSvc AFTER releasing it. -// A watcher-triggered reload of config A interleaving with an API apply of B -// could therefore end with r.cfg=A while configSvc=B permanently — the REST -// surface and the live components disagreeing with no further event to -// reconcile them. +// Several methods commit to BOTH stores but historically did so under r.mu in +// different orders, releasing r.mu between the two writes: +// - ApplyConfig (REST apply path) — r.cfg first, configSvc last. +// - ReloadConfiguration (disk / fsnotify) — configSvc first, r.cfg last. +// - UpdateConfig (registry-source CRUD) — configSvc first, r.cfg last. +// - SaveConfiguration (enable/quarantine/…) — snapshot read, then configSvc +// - r.cfg.Servers. // -// Running the two paths concurrently many times, the two stores must always -// converge to the same value after each round. Before the fix (a shared -// configCommitMu serializing the whole commit in both paths) this fails on -// some round; after it, it always converges. This is a logical lost-update -// race, not a data race, so -race alone won't flag it — the convergence -// assertion is what catches it. -func TestConfigCommit_ApplyAndReloadConverge(t *testing.T) { +// Interleaved, two of these could leave r.cfg and configSvc reporting +// different configs permanently — the REST surface disagreeing with the live +// components with no further event to reconcile them. The fix serializes every +// two-store commit under a shared configCommitMu (acquired outside r.mu). This +// test drives all four paths concurrently and asserts the two stores always +// converge. It is a logical lost-update race, not a data race, so -race alone +// won't flag it — the convergence assertion is what catches it. Before the fix +// this fails on an early round; after it, it always converges. +func TestConfigCommit_CommitPathsConverge(t *testing.T) { tmpDir := t.TempDir() cfgPath := filepath.Join(tmpDir, "mcp_config.json") @@ -47,31 +49,43 @@ func TestConfigCommit_ApplyAndReloadConverge(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { _ = rt.Close() }) + editedWithLimit := func(limit int) *config.Config { + c := config.DefaultConfig() + c.Listen = base.Listen + c.DataDir = base.DataDir + c.ToolResponseLimit = limit + return c + } + const rounds = 200 for i := 0; i < rounds; i++ { - // Disk currently holds the previous round's config; the reload reads - // whatever is on disk, while the apply writes a fresh, distinct value - // (and overwrites disk). Distinct limits make a divergence observable. + // Distinct per-path limits make any divergence between the two stores + // observable regardless of which path commits last. applyLimit := 100000 + i + updateLimit := 200000 + i var wg sync.WaitGroup - wg.Add(2) + wg.Add(4) go func() { defer wg.Done() _ = rt.ReloadConfiguration() }() - go func(limit int) { + go func() { + defer wg.Done() + _, _ = rt.ApplyConfig(editedWithLimit(applyLimit), cfgPath) + }() + go func() { defer wg.Done() - edited := config.DefaultConfig() - edited.Listen = base.Listen - edited.DataDir = base.DataDir - edited.ToolResponseLimit = limit - _, _ = rt.ApplyConfig(edited, cfgPath) - }(applyLimit) + rt.UpdateConfig(editedWithLimit(updateLimit), cfgPath) + }() + go func() { + defer wg.Done() + _ = rt.SaveConfiguration() + }() wg.Wait() - // r.cfg (GetConfig / REST) and configSvc (ConfigSnapshot / subscribers) - // must report the same config once both commits have settled. + // Once all commits have settled, r.cfg (GetConfig / REST) and configSvc + // (ConfigSnapshot / subscribers) must report the same config. legacy, gerr := rt.GetConfig() require.NoError(t, gerr) snap := rt.ConfigSnapshot().Config diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index bae01025b..a30d4652d 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1011,6 +1011,17 @@ func (r *Runtime) LoadConfiguredServers(cfg *config.Config) error { // SaveConfiguration persists the runtime configuration to disk. func (r *Runtime) SaveConfiguration() error { + // Serialize the read-modify-write against the other two-store commit paths + // (ApplyConfig, ReloadConfiguration, UpdateConfig). This reads the current + // configSvc snapshot, splices in the latest servers, then writes both + // configSvc and r.cfg.Servers; without the lock a concurrent ApplyConfig + // could land its new config between the snapshot read and these writes, + // and this stale-based write would clobber configSvc while r.cfg keeps the + // applied value — leaving the two stores divergent (PR #857 review). MUST + // be acquired before r.mu. + r.configCommitMu.Lock() + defer r.configCommitMu.Unlock() + latestServers, err := r.storageManager.ListUpstreamServers() if err != nil { r.logger.Error("Failed to get latest server list from storage for saving", zap.Error(err)) @@ -1116,7 +1127,9 @@ func (r *Runtime) ReloadConfiguration() error { if loadErr != nil { return fmt.Errorf("failed to reload config: %w", loadErr) } - r.UpdateConfig(newConfig, cfgPath) + // Already holding configCommitMu; use the locked helper so we don't + // re-acquire the non-reentrant mutex (would deadlock). + r.updateConfigLocked(newConfig, cfgPath) newSnapshot = r.ConfigSnapshot() } diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 6c59c9b30..29e85145c 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -412,6 +412,19 @@ func (r *Runtime) ConfigPath() string { // UpdateConfig replaces the runtime configuration in-place. // This now updates both the legacy field and the ConfigService. func (r *Runtime) UpdateConfig(cfg *config.Config, cfgPath string) { + // Serialize against the other two-store commit paths (ApplyConfig, + // ReloadConfiguration, SaveConfiguration) so configSvc and the legacy + // r.cfg field can't be updated in an interleaved order that leaves them + // divergent (PR #857 review). MUST be acquired before r.mu. + r.configCommitMu.Lock() + defer r.configCommitMu.Unlock() + r.updateConfigLocked(cfg, cfgPath) +} + +// updateConfigLocked performs the UpdateConfig work assuming the caller already +// holds configCommitMu (e.g. ReloadConfiguration's legacy configSvc==nil +// fallback, which must not re-acquire the non-reentrant mutex). +func (r *Runtime) updateConfigLocked(cfg *config.Config, cfgPath string) { // Update ConfigService first if r.configSvc != nil { if cfgPath != "" { From 8296150c489009153be8cdbf33a6d133d915ee7d Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 22 Jul 2026 08:33:29 +0300 Subject: [PATCH 12/12] fix(runtime): route UpdateListenAddress through the two-store commit protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round-2 review of PR #857 found UpdateListenAddress mutated only r.cfg.Listen (in place, under r.mu) and never touched configSvc, so after any listen change the two config stores diverged on Listen permanently — and because r.cfg shares its pointer with configSvc after UpdateConfig, the in-place write also raced with lock-free ConfigSnapshot readers. Worse, a following SaveConfiguration(persist) cloned the stale configSvc snapshot and persisted the OLD address. UpdateListenAddress now clones the current snapshot, sets Listen, and commits to both stores via updateConfigLocked under configCommitMu (acquired before r.mu) — consistent, race-free, and serialized with the other commit paths. Also close SaveConfiguration's I/O-failure divergence: sync r.cfg.Servers into the legacy store BEFORE SaveToFile (new syncServersToLegacyConfig helper) so a failed disk write leaves the two in-memory stores in agreement rather than configSvc-updated / r.cfg-stale. Extend the regression test to drive UpdateListenAddress concurrently and assert both ToolResponseLimit and Listen converge across all commit paths. --- internal/runtime/config_commit_race_test.go | 17 +++++++++--- internal/runtime/lifecycle.go | 30 ++++++++++++++------- internal/runtime/runtime.go | 22 ++++++++++++--- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/internal/runtime/config_commit_race_test.go b/internal/runtime/config_commit_race_test.go index 3ff54cc80..370c622b4 100644 --- a/internal/runtime/config_commit_race_test.go +++ b/internal/runtime/config_commit_race_test.go @@ -1,6 +1,7 @@ package runtime import ( + "fmt" "path/filepath" "sync" "testing" @@ -64,8 +65,10 @@ func TestConfigCommit_CommitPathsConverge(t *testing.T) { applyLimit := 100000 + i updateLimit := 200000 + i + listenAddr := fmt.Sprintf("127.0.0.1:%d", 40000+i) + var wg sync.WaitGroup - wg.Add(4) + wg.Add(5) go func() { defer wg.Done() _ = rt.ReloadConfiguration() @@ -82,15 +85,23 @@ func TestConfigCommit_CommitPathsConverge(t *testing.T) { defer wg.Done() _ = rt.SaveConfiguration() }() + go func() { + defer wg.Done() + _ = rt.UpdateListenAddress(listenAddr) + }() wg.Wait() // Once all commits have settled, r.cfg (GetConfig / REST) and configSvc - // (ConfigSnapshot / subscribers) must report the same config. + // (ConfigSnapshot / subscribers) must report the same config — both the + // hot-reloadable ToolResponseLimit and the Listen address. legacy, gerr := rt.GetConfig() require.NoError(t, gerr) snap := rt.ConfigSnapshot().Config require.Equal(t, legacy.ToolResponseLimit, snap.ToolResponseLimit, - "round %d: r.cfg and configSvc diverged (r.cfg=%d configSvc=%d)", + "round %d: r.cfg and configSvc diverged on ToolResponseLimit (r.cfg=%d configSvc=%d)", i, legacy.ToolResponseLimit, snap.ToolResponseLimit) + require.Equal(t, legacy.Listen, snap.Listen, + "round %d: r.cfg and configSvc diverged on Listen (r.cfg=%q configSvc=%q)", + i, legacy.Listen, snap.Listen) } } diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index a30d4652d..f764ebdb1 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1054,12 +1054,18 @@ func (r *Runtime) SaveConfiguration() error { zap.Bool("using_config_service", r.configSvc != nil)) // Use ConfigService to save (doesn't hold locks, handles file I/O) + oldServerCount := 0 if r.configSvc != nil { // Update the config service with latest servers first if err := r.configSvc.Update(configCopy, configsvc.UpdateTypeModify, "save_configuration"); err != nil { r.logger.Error("Failed to update config service", zap.Error(err)) return err } + // Keep the legacy r.cfg store in sync with configSvc BEFORE the disk + // write. If SaveToFile then fails we return an error, but the two + // in-memory stores still agree (only disk is stale) — a failed save + // must not leave configSvc and r.cfg divergent (PR #857 review). + oldServerCount = r.syncServersToLegacyConfig(latestServers) // Then persist to disk if err := r.configSvc.SaveToFile(); err != nil { r.logger.Error("Failed to save config to file via config service", zap.Error(err)) @@ -1067,23 +1073,15 @@ func (r *Runtime) SaveConfiguration() error { } r.logger.Debug("Config saved to disk via config service") } else { - // Fallback to legacy save + // Fallback to legacy save (no configSvc store to keep in sync) if err := config.SaveConfig(configCopy, snapshot.Path); err != nil { r.logger.Error("Failed to save config to file (legacy path)", zap.Error(err)) return err } + oldServerCount = r.syncServersToLegacyConfig(latestServers) r.logger.Debug("Config saved to disk via legacy path") } - // Update in-memory config (applies to both configSvc and legacy paths) - r.logger.Debug("Updating in-memory config with latest servers", - zap.Int("server_count", len(latestServers))) - - r.mu.Lock() - oldServerCount := len(r.cfg.Servers) - r.cfg.Servers = latestServers - r.mu.Unlock() - r.logger.Debug("Configuration saved and in-memory config updated", zap.Int("old_server_count", oldServerCount), zap.Int("new_server_count", len(latestServers)), @@ -1095,6 +1093,18 @@ func (r *Runtime) SaveConfiguration() error { return nil } +// syncServersToLegacyConfig writes the latest server list into the legacy +// r.cfg store under r.mu and returns the previous server count. Callers must +// hold configCommitMu so this stays serialized with the other config-commit +// paths. +func (r *Runtime) syncServersToLegacyConfig(latestServers []*config.ServerConfig) int { + r.mu.Lock() + defer r.mu.Unlock() + oldServerCount := len(r.cfg.Servers) + r.cfg.Servers = latestServers + return oldServerCount +} + // ReloadConfiguration reloads the configuration from disk and resyncs state. func (r *Runtime) ReloadConfiguration() error { r.logger.Info("Reloading configuration from disk") diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 29e85145c..3be3ef452 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -456,12 +456,26 @@ func (r *Runtime) UpdateListenAddress(addr string) error { return fmt.Errorf("invalid listen address %q: %w", addr, err) } - r.mu.Lock() - defer r.mu.Unlock() - if r.cfg == nil { + // Commit the listen change to BOTH config stores atomically. Mutating only + // r.cfg.Listen (as this used to) left configSvc's snapshot stale forever — + // and a subsequent SaveConfiguration, which clones that stale snapshot, + // would even persist the OLD address. Routing through updateConfigLocked + // under configCommitMu keeps configSvc and r.cfg in agreement and serializes + // against the other commit paths (PR #857 review). MUST be acquired before + // r.mu. + r.configCommitMu.Lock() + defer r.configCommitMu.Unlock() + + snapshot := r.ConfigSnapshot() + if snapshot == nil || snapshot.Config == nil { return fmt.Errorf("runtime configuration is not available") } - r.cfg.Listen = addr + updated := snapshot.Clone() + if updated == nil { + return fmt.Errorf("failed to clone configuration") + } + updated.Listen = addr + r.updateConfigLocked(updated, "") return nil }