fix(runtime): add config file watcher so external edits hot-reload (HOTRELOAD)#857
Merged
Conversation
…OTRELOAD) 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).
Deploying mcpproxy-docs with
|
| Latest commit: |
8296150
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff5a00e4.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-rc51-hotreload.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 29894129166 --repo smart-mcp-proxy/mcpproxy-go
|
…aves 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.
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.
…loads 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.
…ion too 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.
…elf-write marker pre-save 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).
…ate opt-out beacon to running daemon - 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.
…nd 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.
…ed recent-writes set 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.
Member
Author
|
Review provenance for the merger:
|
… 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.
…igCommitMu too 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.
…protocol 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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Dumbris
added a commit
that referenced
this pull request
Jul 22, 2026
…r pointer (#861) Two live defects surfaced by Codex review of PR #857: 1. Data race: Runtime.Truncator() read r.truncator without holding r.mu while ApplyConfig swapped it under the lock (-race detectable). The field is now an atomic.Pointer[truncate.Truncator]; the accessor and the swap use Load/Store, so reads stay lock-free on the serving hot path and are independent of r.mu (composes with #857's configCommitMu commit-path serialization). 2. Stale captured pointer: the MCP request handler captured the truncator pointer once at construction, so a hot-reloaded tool_response_limit never reached live tool-call responses. NewMCPProxyServer now takes a func() *truncate.Truncator getter; the serving path resolves the truncator at use time via currentTruncator(). Production passes rt.Truncator (method value), so a swap takes effect on the next call. Tests: - TestTruncatorSwapRace: concurrent Truncator() reads + ApplyConfig swaps under -race. - TestTruncatorHotReloadOnServingPath: a limit change is observed by the serving path after reload. Closes #861 (partial): the two live defects only. The broader ApplyConfig/ReloadConfiguration side-effect unification remains tracked in
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 22, 2026
…r pointer (#861) (#887) Two live defects surfaced by Codex review of PR #857: 1. Data race: Runtime.Truncator() read r.truncator without holding r.mu while ApplyConfig swapped it under the lock (-race detectable). The field is now an atomic.Pointer[truncate.Truncator]; the accessor and the swap use Load/Store, so reads stay lock-free on the serving hot path and are independent of r.mu (composes with #857's configCommitMu commit-path serialization). 2. Stale captured pointer: the MCP request handler captured the truncator pointer once at construction, so a hot-reloaded tool_response_limit never reached live tool-call responses. NewMCPProxyServer now takes a func() *truncate.Truncator getter; the serving path resolves the truncator at use time via currentTruncator(). Production passes rt.Truncator (method value), so a swap takes effect on the next call. Tests: - TestTruncatorSwapRace: concurrent Truncator() reads + ApplyConfig swaps under -race. - TestTruncatorHotReloadOnServingPath: a limit change is observed by the serving path after reload. Closes #861 (partial): the two live defects only. The broader ApplyConfig/ReloadConfiguration side-effect unification remains tracked in
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Editing
mcp_config.jsonexternally (editor save,jq > tmp && mv, CLI commands against a running core) never took effect until restart, even though CLAUDE.md and the swagger docs both claim "the file watcher hot-reloads". Only RESTPATCH /api/v1/configworked, because it goes through a different path (ApplyConfig). Found by v0.51.0-rc.1 QA (2026-07-15).Root cause
The watcher never existed.
fsnotifywas only an indirect dependency (go.mod:89) with zero imports; the code itself admitted it (internal/runtime/lifecycle.go:1117,:1426,cmd/mcpproxy/telemetry_cmd.go:294— "there is no fsnotify watcher"). The complete disk-reload pathRuntime.ReloadConfiguration()(internal/runtime/lifecycle.go:1081) existed with no automatic caller, andconfigsvcwas built watcher-ready (Update.Sourceexample "file_watcher",configsvc/service.go:29) but nothing ever fed it.Fix
New
internal/runtime/config_watcher.go, started fromStartBackgroundInitializationand tied to the runtime app context:mv tmp file, atomic-write editors, mcpproxy's ownatomicWriteFile— can never orphan the watch. Covers in-place truncate writes too.Runtime.ReloadConfiguration()(same events / telemetryNotifyConfigChanged/ update-check re-gating / ConnectAll + reindex as manual reload).ApplyConfig/SaveConfigurationsaves never echo back into a redundant reload. No loop risk:ReloadConfigurationnever writes the file.ReloadFromFilefails before mutating state); the watcher survives and picks up the next valid write.Also: promoted
fsnotify v1.9.0indirect → direct (already in go.sum, no new third-party code), updated the three stale "there is no fsnotify watcher" comments, and documented debounce / invalid-JSON / restart-required semantics indocs/configuration.md.Known limitations (accepted): a config-path change after startup is not re-watched; restart-required fields (
listen,data_dir) edited externally reload into memory but still need a restart — same semantics as the manual reload path.Tests
New
internal/runtime/config_watcher_test.go(written first, TDD — failed withstartConfigFileWatcher undefinedpre-fix):TestConfigWatcher_RenameWriteTriggersReload— atomic tmp+rename edit hot-reloadsTestConfigWatcher_TruncateWriteTriggersReload— in-place>edit hot-reloadsTestConfigWatcher_InvalidJSONKeepsOldConfigThenRecovers— bad JSON keeps old config + version; watcher recoversTestConfigWatcher_SelfWriteSuppressed—ApplyConfig's own save produces nofile_reloadechoTestConfigWatcher_DebounceCoalesces— 10 rapid writes collapse to ≤3 reloads, final value landsTestConfigWatcher_MissingDirGracefulDegradation— missing parent dir errors without panicCommands run:
go test -race ./internal/runtime/...— all passgo test -race ./cmd/mcpproxy/...— passgo test -tags server ./internal/serveredition/... -race— passgo build ./cmd/mcpproxyandgo build -tags server ./cmd/mcpproxy— both compilegolangci-lint run --config .github/.golangci.yml ./internal/runtime/... ./cmd/mcpproxy/...— 0 issues