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") +}