Fall back to Legacy on an inconclusive vMCP revision probe

[jhrozek]

Summary

ListCapabilities is both the vMCP health-check probe (pkg/vmcp/health/checker.go:99) and the startup aggregation call (pkg/vmcp/aggregator/default_aggregator.go:95). Since 885e8b1 it routes through dispatch, which resolves the backend's MCP revision by probing server/discover first and treated any probe error as fatal.

probeRevision deliberately returns its error uncached for the two inconclusive classes — errModernAuth (401/403/407) and errModernTransient (408/429/5xx, mid-stream read failure, or any transport error) — precisely because those outcomes say nothing about the backend's revision. Aborting on them meant a backend that was up and serving Legacy got reported unhealthy, which the existing readiness machinery amplified into delayed vMCP Ready and an empty aggregated tool list.

This restores the pre-885e8b14f outcome: fall back to Legacy, the pre-dual-era default, without caching the revision.

• A transient outage can no longer pin a backend to Legacy.
• A genuinely Modern backend is corrected by the next call's re-probe.
• A truly-down backend still fails fast — the Legacy initialize hits the same dead socket.

The fallback lives in dispatch, not probeRevision, on purpose: probeRevision must keep returning inconclusive errors uncached so the revision cache is never poisoned. Everything after err := fn(ctx, rev) — the reclassify-on-mismatch block — is unchanged.

Closes #6000

Type of change

• Bug fix (non-breaking change that fixes an issue)

Changes

File	Change
pkg/vmcp/client/client.go	dispatch falls back to RevisionLegacy uncached on a probe error instead of returning wrapBackendError(..., "probe revision"); doc comments on dispatch and probeRevision updated to match
pkg/vmcp/client/dispatch_legacy_fallback_test.go	New regression test

Test plan

• Unit tests pass (task test)
• New regression test added

TestDispatch_LegacyFallbackOnInconclusiveProbe drives one httptest.Server that discriminates on the Mcp-Method header: the Modern probe gets HTTP 503 (→ errModernTransient), while the Legacy initialize + tools/list path succeeds. It asserts ListCapabilities returns the tool, and that the revision cache is not pinned afterwards.

Verified as a negative control — with the production hunk reverted, the new test fails with exactly the bug's fingerprint:

backend unavailable: failed to probe revision for backend flaky-probe:
modern backend returned a transient error: HTTP 503

Also confirmed still green: TestProbeRevision_TransientLeavesUnprobed, TestDispatch_TransientDoesNotReclassify, TestProbeRevision_TruthTable, TestRegression_401_*, TestRegression_403OnInitialize_*, TestRegression_ToolSchemaFidelity_PreservesCompositors, TestListCapabilities_*.

Special notes for reviewers

Scope — please read. This does not fix the dial tcp …: connect: connection refused signature seen in some failing e2e runs. There the socket is genuinely dead and the Legacy initialize fails identically; the health monitor's retry interval absorbs it. What this removes is the 5xx/408/429/auth window that 885e8b1 made fatal — concretely, a ToolHive proxy that is listening while its MCP container is still starting answers the probe with 502/503, and the probe consumes that window and aborts before the Legacy path is ever attempted. I'd rather state that precisely than claim it eliminates every vMCP e2e flake.

Why the fallback is unconditional. It triggers on any probeRevision error, including the "failed to build transport" misconfiguration class. defaultClientFactory (client.go:659) calls the same buildBackendRoundTripper, so that class fails identically on the Legacy path — only the operation label in the error changes ("create client" vs "probe revision"). Narrowing it to the two sentinels would add a branch that distinguishes nothing observable.

Safety checks done:

• No double execution. dispatch is shared with CallTool, but in the uncached branch if !cached { return err } short-circuits before the retry machinery, so fn runs at most once. legacyCallTool also runs legacyInit first (client.go:1572) and returns early on failure, so a genuinely Modern backend rejects the Legacy initialize before c.CallTool (client.go:1590) is reachable.
• No protocol downgrade. Both eras build their transport through the same buildBackendRoundTripper (client.go:560) and the same newBackendHTTPClient choke point (client.go:1010), so auth injection, TLS/CA, SameHostRedirectPolicy and the SSRF dial hook are identical. There is no weaker Legacy path to downgrade to.
• Health classification unchanged. The 401/403-with-outgoing-auth → BackendHealthy mapping in categorizeError/authErrorStatus is untouched.

Known cost: against a permanently-misconfigured or down backend, the fallback adds one extra attempt per call (transport construction is CPU-only and repeated, since the failure is deliberately never cached). The fallback shares the probe's context, so it inherits the remaining health-check budget rather than a fresh one — harmless for the case being fixed, where the probe returns immediately.

Out of scope and deliberately untouched: the readiness/health-filtering amplifiers in pkg/vmcp/server/status_reporting.go, pkg/vmcp/health/monitor.go and pkg/vmcp/core/core_vmcp.go (ListTools) — pre-existing, tracked in #3100.

This is the third and last fix for 885e8b1 fallout, after #5999.

cc @JAORMX (owner of the related migration PR #5993)

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.04%. Comparing base (885e8b1) to head (36a9802).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6001      +/-   ##
==========================================
- Coverage   72.06%   72.04%   -0.02%     
==========================================
  Files         718      718              
  Lines       74462    74465       +3     
==========================================
- Hits        53662    53652      -10     
- Misses      16965    16977      +12     
- Partials     3835     3836       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

LGTM. Tight, well-scoped fix for the last of the 885e8b1 fallout.

The core change is correct: on any probeRevision error in the uncached path, dispatch falls back to RevisionLegacy uncached rather than treating the probe as fatal. Keeping the fallback in dispatch and not in probeRevision is the right call — probeRevision stays free to return inconclusive errors uncached so the revision cache is never poisoned, and the next call re-probes once the outage clears.

Verified the safety reasoning holds:

• No double execution — the uncached branch still hits if !cached { return err } before the retry machinery, so fn runs at most once; a genuinely Modern backend rejects the Legacy initialize early.
• No protocol downgrade — both eras build transport through the same buildBackendRoundTripper / newBackendHTTPClient choke point, so auth/TLS/SSRF-dial handling is identical.
• Truly-down backend still fails fast — the Legacy initialize hits the same dead socket.

The regression test is exactly right: it drives the probe (via newProbeClient, not setRevision) so the Modern 503 → errModernTransient path actually runs, asserts the tool is returned, and asserts the revision cache stays unpinned. Negative-control verification with the production hunk reverted is a nice touch, and using newProbeClient sidesteps the nil-registry trap from #5990.

Appreciate the honest scoping note that this doesn't claim to eliminate the connection refused e2e signature — only the 5xx/408/429/auth probe window. Approving; will hold on merge for green CI + your call.