Repository navigation
fix(proxy): follow a replaced daemon instead of dying with it - #11
Merged
Merged
Conversation
Proxy read the daemon's port once. After a restart (a deploy, a crash, an idle shutdown) the next request failed and Proxy returned, so every MCP client had to reconnect by hand; the lease was not re-attached either, so the new daemon saw zero agents while clients were still open. - A dropped lease makes the proxy poll the PID file for up to Config.Timeout and re-attach. It never starts a daemon there: Stop, Hold and the idle timer must not be undone by an idle client. - A request whose dial failed, or that a daemon answered 401 (another daemon on the old port), never ran: it is sent once more after reconnecting (EnsureRunning when Config.Binary is set, else the PID file). - A request that failed after it was sent may have run: the client gets a JSON-RPC error for its id (-32603) and it is never sent again. Notifications get no answer. - Proxy returns only on context end, stdin end/error or stdout error. Tests: integration (helper-process daemon) for restart, lease re-attach under a 2 s idle timeout, on-demand start after Stop, mid-flight kill (executed exactly once), Hold/Release; unit tests for the dial/401 classification and the error reply. All new tests fail on v0.4.0; five mutations (no re-attach, no retry, retry everything, watcher starts the daemon, no 401 retry) each fail a test.
golangci-lint (latest, as CI runs it) flags receives from error channels whose value is discarded. The serve tests now require Serve to return nil after cancel, and the attached-clients test requires Proxy to return nil once stdin is closed and its ctx cancelled (the daemon being gone no longer ends it). In Group.Run the remaining actors' errors are drained on purpose: the first error decides the result; nolint with that reason.
windows-latest CI failed two of the new proxy tests. The lease watcher polled pidlock.IsHeld every 200 ms, and IsHeld opens the PID file for writing: on Windows that holds it exclusively for a moment. A daemon starting at that moment failed with "already running: pid file locked", and Status read the stale file of a stopped daemon as running. With many proxies watching during a deploy, a restart could fail the same way. The watcher and reconnect now only read the PID file (a shared read) and let the lease prove the daemon alive: a stale file names a dead port (attach refused), a foreign daemon on that port rejects the token (401). Regression test: while a proxy watches and no daemon runs, a tight TryLock loop never finds the lock taken (red before: 2 and 3 hits in ~170k tries), and Start succeeds. Mutation (IsHeld back in the watcher) fails it.
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
1 participant
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
Proxyread the daemon's port once at start. When the daemon was replaced while a client was connected (a restart during a deploy, a crash, an idle shutdown):connection refused,Proxyreturned the error, and the host process exited, so every MCP client had to reconnect by hand;Fix
The proxy now follows a replaced daemon:
Config.Timeoutand re-attaches. It never starts a daemon here, soStop,Holdand the idle timer are not undone by an idle clientEnsureRunningwhenConfig.Binaryis set, otherwise the PID file) and send it once more-32603, "daemon unavailable: …") and the request is never sent again. Notifications get no answerReleaseProxynow returns only when its context ends, stdin ends or fails, or stdout fails.proxyAttachreturns a lease (cancel + done channel) instead of a bare cancel func, so the watcher can see the connection end. The public API is unchanged.Tests
go test ./...: ok;go vetfor windows/linux/darwin: ok;golangci-lint run ./...(whole repo) for windows/linux/darwin: 0 issues;Found while running GODE: every deploy forced
/mcpreconnect in every session.Also in this PR
test:the serve tests requireServeto return nil after cancel, and the attached-clients test requiresProxyto return nil once stdin closes. golangci-lint (latest, as CI runs it) flagged the discarded receives.chore: prepare release v0.4.1: CHANGELOG and ROADMAP.fix(proxy): look for the next daemon without probing the PID lock: windows-latest CI showed that pollingpidlock.IsHeldholds the PID file exclusively for a moment on Windows, so a starting daemon failed with "already running" andStatusread a stale file as running. The watcher now only reads the PID file and lets the lease prove the daemon alive. A regression test checks that a tightTryLockloop never finds the lock taken while a proxy is watching (before the fix it hit 2–3 times in ~170k tries).