Repository navigation
fix: disabling a server removes its tools from search again (regression from #1590) - #1603
Merged
Merged
Conversation
…1590) TestE2E_DisableServerRemovesToolsFromSearch has failed on main since Spec 116 PR1 at its re-enable step (e2e_test.go:2903): disable purges the server's tools correctly, but after re-enable the server stays "Ready" with no tools. Root cause. Every config commit now publishes to configsvc, so the supervisor reconciles a change the commit's own worker is already applying. Its snapshot still holds the disabled entry, so it plans ActionReconnect, whose first leg DisconnectServer(name) resolves the client by name and disconnects the instance the commit worker had just installed and was connecting. managed.Client.Connect then finished with TransitionTo(Ready) although that Disconnect had closed the transport and reset the state, so the client reported Ready over a closed transport: every ListTools failed with "client not connected" and, since the client looked connected, nothing reconnected it. Fix. - supervisor: ActionReconnect no longer disconnects by name. AddServer (Manager.AddServerConfigAt) already replaces and disconnects, atomically under the manager lock, an instance whose connection config differs from the desired one, using the same ConnectionEquivalent predicate that plans the reconnect; an instance already holding the desired config is kept. Spec 116 PR3 still binds the remaining by-name supervisor actions to (generation, instance). - managed: Connect returns ErrConnectAborted and stays Disconnected when a Disconnect ran during the dial, instead of reporting Ready; the manager treats that as superseded, not failed. Tests (failing first): supervisor TestReconnect_DoesNotDisconnectTheCommittedInstance, managed TestConnect_DisconnectDuringDialDoesNotReportReady (reproduces the exact "invalid transition from Disconnected to Ready" log line via a dial seam). TestSupervisor_Reconcile_DisableServer's mock now models the manager's replace-on-non-equivalent-config behaviour.
Deploying mcpproxy-docs with
|
| Latest commit: |
eb85784
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f09bfd1a.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-disable-server-purges-se.mcpproxy-docs.pages.dev |
… state (review r1) The abort check looked only at a successful dial finding the state Disconnected. A Disconnect that cancelled the dial made it fail, and the failure branch overwrote the reset state with Error/PendingAuth; a newer Connect that re-entered Connecting (or reached Ready) also defeated the state check, letting the older attempt publish over it. Connect now takes a new connectSeq under mc.mu when it starts and every Disconnect advances it; phase 4 discards the attempt's result, success or failure, when the sequence moved, and closes the dialed transport only when no newer attempt owns the shared core client. Tests cover the failing-dial and newer-attempt cases (both fail on the state-only check).
…review r2) tryReconnect and TryReconnectSync tear the connection down without mc.mu and then start their own Connect. An older attempt whose dial finished in between saw an unchanged connectSeq and published Ready over the closed transport, so the helper's Connect hit ErrConnectAlreadyActive and the client stayed Ready over a closed transport. Both helpers now advance connectSeq under mc.mu before the teardown. Regression test parks an attempt after its dial and releases it between the helper's teardown and its replacement Connect (fails without the supersede).
The reconnect helpers bumped connectSeq under mc.mu but then cancelled, closed the transport and reset the state outside it, so a Connect from another caller (ConnectAll, the supervisor) admitted in between captured the new sequence and could publish Ready over the transport the teardown then closed. teardownForReconnect now does the bump, the transport close and the reset in one mc.mu section, as Disconnect does, which orders Connect admission (phase 1, under mc.mu) against the whole teardown. Adds coverage for an attempt admitted after the teardown.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 38295257002 --repo smart-mcp-proxy/mcpproxy-go
|
… (review r4) Round 4 noted the round-3 test runs its competing Connect after the teardown and so also passes with the non-atomic teardown. A seam between the transport close and the state reset now asserts mc.mu (the lock Connect admission takes) is held there, for both reconnect helpers; it fails against the round-3 shape.
…ions reconcile dispatches its actions asynchronously; on the Windows runner the initial ActionConnect from the old snapshot ran after the explicit ActionReconnect and re-installed the old config, failing TestReconnect_ReplacesAnInstanceWithAnOldConnectionConfig. Wait on actionWg after each reconcile in both new tests.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Summary
mainhas been red in E2E Tests since #1590 (Spec 116 PR1, 43c5dc8).TestE2E_DisableServerRemovesToolsFromSearchfails ate2e_test.go:2903. Disabling the server does remove its tools: Step 3 passes in CI and locally. The failure is the re-enable step. The server comes back as "Ready" with no tools, and nothing reconnects it until the 30 s periodic reconcile or later. Locally on main it failed 2 of 3 runs; it passes on 4cc4bcb.Root cause
configsvc, and its own worker (runCommitWorkers→Manager.AddServerAt) installs and connects the re-enabled instance. The supervisor reconciles the same publish. Its snapshot still holds the disabled entry, so it plansActionReconnect. The first leg of that action isDisconnectServer(name), which resolves the client by name, so it disconnected the instance the commit worker was still connecting. (Before fix: single config commit section with per-server generations (Spec 116 PR1; #1558, #1560) #1590 the MCP path published only after the connect had finished, so the reconnect was redundant but harmless.)managed.Client.Connectreleasesmc.muwhile it dials. A Disconnect in that window closes the transport and resets the state to Disconnected, but Connect then still ranTransitionTo(Ready). The log showsinvalid transition from Disconnected to Ready. The client reported Ready, everyListToolsfailed withclient not connected, and because the client looked connected the supervisor never reconnected it.Fix
ActionReconnectno longer disconnects by name.AddServer→Manager.AddServerConfigAtalready replaces and disconnects a non-equivalent instance, atomically under the manager lock, using the sameconfig.ConnectionEquivalentpredicate that plans the reconnect. An instance that already holds the desired config is kept. Spec 116 PR3 (bug: stale connect/disconnect workers and supervisor actions re-install old server configs after a newer commit #1563) still binds the remaining by-name supervisor actions to (generation, instance).Connectnow takes a newconnectSeqvalue (guarded bymc.mu) when it starts, andDisconnectadvances it. Phase 4 discards the result of a superseded attempt, success or failure, and returnsErrConnectAborted; it closes the dialed transport only when no newer attempt owns the shared core client. The reconnect helpers (tryReconnect,TryReconnectSync) now tear down throughteardownForReconnect, which does the sequence bump, the transport close and the state reset in onemc.musection, asDisconnectdoes.Manager.AddServerAttreatsErrConnectAbortedas "superseded", not as a failure.Spec 116's commit model is unchanged.
Tests (each failed before the fix)
supervisor/TestReconnect_DoesNotDisconnectTheCommittedInstance: the instance installed by the commit with the desired config must not be disconnected by a stale-snapshotActionReconnect.TestReconnect_ReplacesAnInstanceWithAnOldConnectionConfigkeeps the reconnect's purpose.managed/TestConnect_DisconnectDuringDialDoesNotReportReady: a dial seam injects a Disconnect between the transport connect and phase 4, which reproduces the exactDisconnected → Readylog line. Connect must returnErrConnectAborted, and the next connect must work.managed/TestConnect_DisconnectDuringFailingDialKeepsDisconnected,TestConnect_NewerAttemptSupersedesTheOlderOne,TestConnect_ReconnectTeardownSupersedesADialingAttempt,TestConnect_AttemptAdmittedAfterReconnectTeardownOwnsTheClientandTestReconnectTeardown_ExcludesConnectAdmissioncover the supersession cases raised in cross-review. Each fails against the previous round's code.TestSupervisor_Reconcile_DisableServernow models the manager's replace-on-non-equivalent-config behaviour, which is how a disable reaches a connected instance now.Verification
go test -race -count=20 -run TestE2E_DisableServerRemovesToolsFromSearch ./internal/server/: 20/20 pass (main: 2/3 fail).e2e-tests.ymlruns it, and the CI-shaped-race -tags serverlane overinternal/server/... internal/runtime/... internal/httpapi/... internal/upstream/...: both pass..github/.golangci.yml), bare and--build-tags server: no findings in the touched files.make build, scratch port 19110,trusted-librarywith 320 tools): disabling via REST and via MCPupstream_servers patchleaves 0 tools inretrieve_tools,describe_tooland REST/index/search, both immediately and after 30 s. Re-enabling restores 20/20 retrieve results,describe_tooland 100 index hits within 3 s, and they are still there after 33 s.Cross-review
OpenCode gpt-6.1-sol (variant high): rounds 1-4 raised findings (three medium, one low) in the managed-client supersession logic, and all were fixed. Round 5 returned VERDICT: PASS. Round 6 reviewed the Windows-only test-ordering fix and returned VERDICT: PASS.
🤖 Generated with Claude Code