diff --git a/cmd/mcpproxy/telemetry_cmd.go b/cmd/mcpproxy/telemetry_cmd.go index 8623c6d3..25771afd 100644 --- a/cmd/mcpproxy/telemetry_cmd.go +++ b/cmd/mcpproxy/telemetry_cmd.go @@ -290,13 +290,20 @@ 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 - // 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. 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. 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 { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() diff --git a/cmd/mcpproxy/telemetry_cmd_test.go b/cmd/mcpproxy/telemetry_cmd_test.go index 7bc4f99f..409183d6 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,76 @@ 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_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) + + if err := runTelemetryDisable(nil, nil); err != nil { + t.Fatalf("runTelemetryDisable: %v", err) + } + if got := atomic.LoadInt32(hits); got != 1 { + t.Errorf("beacon sends = %d, want 1", got) + } + // The disable itself must be persisted. + enabled := readTelemetryEnabled(t, configFile) + if enabled == nil || *enabled { + t.Errorf("telemetry.enabled must be persisted false") + } +} diff --git a/docs/configuration.md b/docs/configuration.md index b52a6ee5..88c8162e 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 786c32fb..d31c867a 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_commit_race_test.go b/internal/runtime/config_commit_race_test.go new file mode 100644 index 00000000..370c622b --- /dev/null +++ b/internal/runtime/config_commit_race_test.go @@ -0,0 +1,107 @@ +package runtime + +import ( + "fmt" + "path/filepath" + "sync" + "testing" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + + "github.com/stretchr/testify/require" +) + +// 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: +// - configSvc (served by ConfigSnapshot / Config() / live subscribers), and +// - the legacy r.cfg field (served by GetConfig / the REST /api/v1/config +// handlers). +// +// 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. +// +// 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") + + 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() }) + + 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++ { + // Distinct per-path limits make any divergence between the two stores + // observable regardless of which path commits last. + applyLimit := 100000 + i + updateLimit := 200000 + i + + listenAddr := fmt.Sprintf("127.0.0.1:%d", 40000+i) + + var wg sync.WaitGroup + wg.Add(5) + go func() { + defer wg.Done() + _ = rt.ReloadConfiguration() + }() + go func() { + defer wg.Done() + _, _ = rt.ApplyConfig(editedWithLimit(applyLimit), cfgPath) + }() + go func() { + defer wg.Done() + rt.UpdateConfig(editedWithLimit(updateLimit), cfgPath) + }() + go func() { + 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 — 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 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/config_watcher.go b/internal/runtime/config_watcher.go new file mode 100644 index 00000000..f588d39e --- /dev/null +++ b/internal/runtime/config_watcher.go @@ -0,0 +1,276 @@ +package runtime + +import ( + "bytes" + "context" + "encoding/json" + "os" + "path/filepath" + "time" + + "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, +// 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) + } + } +} + +// 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 +// 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). 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() + 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 +} + +// 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() + r.pruneExpiredSelfWritesLocked(time.Now()) + for _, e := range r.recentSelfWrites { + if bytes.Equal(e.payload, trimmedDisk) { + return true + } + } + return false +} + +// 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 saves, and keeping the stale records would suppress a +// later external revert to those exact bytes (editor undo, `git checkout`). +func (r *Runtime) clearSelfWrites() { + r.selfWriteMu.Lock() + 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 +// 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 { + // 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. + 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 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 markers so a later external re-write of + // those old self-saved bytes still reloads. + if !r.matchesRecentSelfWrite(trimmedDisk) { + r.clearSelfWrites() + } + 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 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 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 { + // 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 00000000..2261cf33 --- /dev/null +++ b/internal/runtime/config_watcher_test.go @@ -0,0 +1,459 @@ +package runtime + +import ( + "context" + "os" + "path/filepath" + "strings" + "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_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_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_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 +// 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_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) { + 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()) +} + +// 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") +} + +// 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") +} + +// 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/lifecycle.go b/internal/runtime/lifecycle.go index f1ec0ffd..f764ebdb 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() } @@ -1004,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)) @@ -1036,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)) @@ -1049,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)), @@ -1077,10 +1093,32 @@ 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") + // 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() @@ -1099,7 +1137,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() } @@ -1107,6 +1147,40 @@ 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() + } + + // 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) + } + + // 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) @@ -1114,8 +1188,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 +1498,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) diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 82e6b4a6..3be3ef45 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -10,6 +10,7 @@ import ( "net" "os" "os/exec" + "reflect" "sort" "strings" "sync" @@ -63,6 +64,32 @@ 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 + // 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 statusCh chan Status @@ -385,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 != "" { @@ -416,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 } @@ -1332,6 +1386,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 @@ -1364,8 +1425,27 @@ 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, 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 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. + // 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)) @@ -1407,30 +1487,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 @@ -2497,6 +2557,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 { diff --git a/internal/upstream/manager.go b/internal/upstream/manager.go index 3b3b6eec..0652b7cf 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)