fix(restart): re-read mcp_config.json before restarting upstream (#467)#471
Open
Dumbris wants to merge 1 commit into
Open
fix(restart): re-read mcp_config.json before restarting upstream (#467)#471Dumbris wants to merge 1 commit into
Dumbris wants to merge 1 commit into
Conversation
Runtime.RestartServer pulled the server's config from BoltDB, never from disk. There is no fsnotify-style auto file-watcher, so a user who edited mcp_config.json (changing env, headers, args, or isolation_json) and then ran `mcpproxy upstream restart` from the CLI / tray would silently replay the stale config — only the live REST PATCH path used to update those maps. The repro from #467 (env wiped + restored on obsidian-pilot) confirmed this: editing the file changed nothing on the running container; only `mcpproxy upstream patch` made the change visible. Fix: pull the latest server config from disk first, fall back to BoltDB on read / parse failure, and persist the disk-loaded config back to BoltDB so subsequent restarts see consistent state. Added lookupServerConfigForRestart() to keep the restart path readable. RestartAll inherits the fix automatically because it loops through RestartServer. Tests cover env, headers, and the storage fallback path. The latter proves a malformed disk file doesn't break restart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
253a921
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://050a3d35.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-467-file-watcher-env-hea.mcpproxy-docs.pages.dev |
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.
Closes #467.
Summary
Runtime.RestartServerpulled the server's config from BoltDB only. There is no fsnotify-style auto file-watcher anywhere in the codebase (despite the issue title — the user's mental model was that one existed). So a user who editedmcp_config.json(changingenv,headers,args, orisolation_json) and then ranmcpproxy upstream restartfrom the CLI / tray silently replayed the stale config — only the live REST PATCH path used to update those maps.The repro from #467 (env wiped + restored on
obsidian-pilot) confirmed this: editing the file changed nothing on the running container; onlymcpproxy upstream patchmade the change visible.Approach
Pull the latest server config from disk first, fall back to BoltDB on read / parse failure, and persist the disk-loaded config back to BoltDB so subsequent restarts (and any other read-from-storage code path) see consistent state. The new
lookupServerConfigForRestart()keepsRestartServeritself readable.RestartAllinherits the fix automatically because it loops throughRestartServer.Why this and not a real fsnotify watcher
Adding a real file watcher is a separate spec — it has correctness concerns around debouncing, atomic-rename-vs-truncate, partial writes, and what to do with the existing
r.cfgsnapshot during reload. The fix here addresses the user-observable gap with a much smaller surface.What about other restart-time staleness?
AddServerConfig(internal/upstream/manager.go:269-276) already diffsEnv,Headers,Argsetc. against the running client's config. So once the disk-loaded config reachesAddServer, the existing reconnect path picks it up correctly. No change needed there.Test plan
go test ./internal/runtime/ -run TestRestartServer -race -v— three new cases:TestRestartServer_PicksUpDiskEnvChanges— reproduces the original repro, asserts BoltDB reflects the disk env after restartTestRestartServer_PicksUpDiskHeaderChanges— same shape for HTTPHeadersTestRestartServer_FallsBackToStorageWhenDiskMissing— guarantees a malformed disk file doesn't break restartgo build ./...clean🤖 Generated with Claude Code