From 36a98029c8d0e8f9555a568b784255f17547742b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sun, 26 Jul 2026 22:07:04 +0200 Subject: [PATCH] Fall back to Legacy on inconclusive vMCP probe ListCapabilities is both the vMCP health-check probe and the startup aggregation call, and since the dual-protocol merge it routes through dispatch, which probes the backend's MCP revision first and treated any probe error as fatal. An inconclusive probe -- a 401/403 blip, an HTTP 408/429/5xx, a timeout, or a connection error -- says nothing about the backend's revision, so failing the whole call marked backends that were up and serving Legacy as unhealthy. That in turn delayed vMCP readiness and emptied the aggregated tool list. Fall back to Legacy, the pre-dual-era default, without caching the revision. A transient outage can no longer pin a backend to Legacy, and a genuinely Modern backend is corrected by the next call's re-probe. A truly-down backend still fails fast, because the Legacy initialize hits the same dead socket. The fallback lives in dispatch rather than probeRevision on purpose: probeRevision must keep returning inconclusive errors uncached so the revision cache is never poisoned. Closes #6000 Co-Authored-By: Claude Opus 5 --- pkg/vmcp/client/client.go | 29 +++++- .../client/dispatch_legacy_fallback_test.go | 94 +++++++++++++++++++ 2 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 pkg/vmcp/client/dispatch_legacy_fallback_test.go diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index e123361c35..90cc40044c 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -1053,7 +1053,9 @@ func (h *httpBackendClient) modernDiscover( // leaving it unprobed lets the next call re-probe once the outage clears. // // A hard error is also returned when the backend transport cannot be built at all -// (e.g. invalid auth/CA config); that is a genuine misconfiguration. +// (e.g. invalid auth/CA config); that is a genuine misconfiguration. Note that +// dispatch, the sole caller, does not distinguish these error classes — it falls +// back to Legacy uncached on any error this function returns. // The resolved capabilities are intentionally not returned: callers that need // them (ListCapabilities) re-fetch via modernDiscover, so probeRevision only // classifies and caches the revision. @@ -1382,6 +1384,11 @@ func (*httpBackendClient) legacyInit( // and runs fn with it. Every call verb AND ListCapabilities route through this // seam so revision selection — and self-correction — lives in one place. // +// A probeRevision error of any kind (inconclusive probe or transport-build +// failure) falls back to Legacy — the pre-dual-era default — WITHOUT caching, +// so a genuinely Modern backend re-probes on the next call instead of being +// pinned to Legacy by one bad probe. +// // On a revision mismatch (isRevisionMismatch), dispatch always re-probes // authoritatively and flips the cache so future calls use the corrected // revision. It RETRIES fn only when the failure proves the backend did NOT @@ -1391,8 +1398,10 @@ func (*httpBackendClient) legacyInit( // cache is flipped but fn is NOT re-run. The retry is unconditionally single // (no re-check), so it can never loop. // -// When the revision was just probed in THIS call (uncached), a re-probe would -// return the same answer, so a mismatch is surfaced directly without re-probing. +// When the revision was resolved in THIS call (uncached), a mismatch is surfaced +// directly without re-probing: on a successful probe a re-probe would just repeat +// the same answer, and on the Legacy fallback above there is no cached revision to +// correct — the next call re-probes anyway once the outage clears. func (h *httpBackendClient) dispatch( ctx context.Context, target *vmcp.BackendTarget, fn func(ctx context.Context, rev mcpparser.Revision) error, @@ -1401,9 +1410,19 @@ func (h *httpBackendClient) dispatch( if !cached { probed, err := h.probeRevision(ctx, target) if err != nil { - return wrapBackendError(err, target.WorkloadID, "probe revision") + // A failed probe (auth blip / transient 5xx / timeout / connection + // refused, or a transport that could not be built) says nothing about + // the revision and must NOT fail a backend that is reachable on the + // Legacy path — which re-validates the transport itself. Fall back to Legacy — + // the pre-dual-era default — WITHOUT caching, so a transient outage + // never pins a revision and a genuinely Modern backend is corrected on + // the next call's re-probe. + slog.DebugContext(ctx, "revision probe inconclusive; attempting Legacy uncached", + "backend", target.WorkloadID, "probe_error", err) + rev = mcpparser.RevisionLegacy + } else { + rev = probed } - rev = probed } err := fn(ctx, rev) diff --git a/pkg/vmcp/client/dispatch_legacy_fallback_test.go b/pkg/vmcp/client/dispatch_legacy_fallback_test.go new file mode 100644 index 0000000000..a22c503a49 --- /dev/null +++ b/pkg/vmcp/client/dispatch_legacy_fallback_test.go @@ -0,0 +1,94 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// TestDispatch_LegacyFallbackOnInconclusiveProbe pins the fix for the +// dispatch abort regressed by merge 885e8b14f: a Modern server/discover probe +// that comes back transient (HTTP 503, -> errModernTransient) must fall back +// to the Legacy initialize/tools-list path uncached, not fail the whole call. +// Caching the fallback would pin a genuinely Modern backend to Legacy past a +// transient outage, so the revision cache must remain unset afterward. +func TestDispatch_LegacyFallbackOnInconclusiveProbe(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Mcp-Method") != "" { + // Modern probe: transient failure, discovered before the body is read. + w.WriteHeader(http.StatusServiceUnavailable) + return + } + + body, err := readAll(t, r) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + var req jsonRPCRequest + if err := json.Unmarshal(body, &req); err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + + w.Header().Set("Content-Type", "application/json") + switch req.Method { + case "initialize": + resp := jsonRPCResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{ + "protocolVersion": "2024-11-05", + "capabilities": {"tools": {}}, + "serverInfo": {"name": "flaky-probe", "version": "1.0.0"} + }`), + } + _ = json.NewEncoder(w).Encode(resp) + case "tools/list": + resp := jsonRPCResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{ + "tools": [{"name": "echo", "description": "echoes input"}] + }`), + } + _ = json.NewEncoder(w).Encode(resp) + default: + _ = json.NewEncoder(w).Encode(jsonRPCResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{}`), + }) + } + })) + t.Cleanup(srv.Close) + + // newProbeClient, not setRevision: the probe must actually run so the + // Modern 503 is hit and the fallback path exercised. + h := newProbeClient(t) + target := &vmcp.BackendTarget{ + WorkloadID: "flaky-probe", + BaseURL: srv.URL, + TransportType: "streamable-http", + } + + caps, err := h.ListCapabilities(context.Background(), target) + require.NoError(t, err) + require.Len(t, caps.Tools, 1) + assert.Equal(t, "echo", caps.Tools[0].Name) + + _, ok := h.cachedRevision("flaky-probe") + assert.False(t, ok, "an inconclusive probe must not pin a revision") +}