Skip known-bad backends when opening vMCP sessions

pkg/vmcp/core/core_vmcp.go

@@ -795,9 +795,9 @@ func filterHealthyBackends(backends []vmcp.Backend, healthStatusProvider health.
 
 		// Include healthy, degraded, and empty/zero-value (assume healthy) backends.
 		// Explicitly exclude unhealthy, unknown, and unauthenticated backends.
-		if healthStatus == "" ||
-			healthStatus == vmcp.BackendHealthy ||
-			healthStatus == vmcp.BackendDegraded {
+		// The predicate is shared with session establishment's stricter
+		// counterpart — see health.ShouldAdvertise / health.ShouldOpenSession.
+		if health.ShouldAdvertise(healthStatus) {
 			healthy = append(healthy, *backend)
 		} else {
 			excluded++

pkg/vmcp/health/policy.go

@@ -0,0 +1,64 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package health
+
+import "github.com/stacklok/toolhive/pkg/vmcp"
+
+// ShouldAdvertise reports whether a backend in this status may contribute
+// capabilities to the advertised view (tools/list and friends).
+//
+// Degraded backends are included: they are slow but working, and hiding their
+// tools would be a worse outcome for the caller than serving them. An empty
+// status means health monitoring is disabled, which is treated as healthy so a
+// deployment without a monitor behaves as it did before monitoring existed.
+//
+// Excluded: unhealthy (not responding), unknown (not yet probed), and
+// unauthenticated (operator misconfiguration).
+func ShouldAdvertise(status vmcp.BackendHealthStatus) bool {
+	return status == "" ||
+		status == vmcp.BackendHealthy ||
+		status == vmcp.BackendDegraded
+}
+
+// ShouldOpenSession reports whether a new session should attempt to open a
+// connection to a backend in this status.
+//
+// This is deliberately STRICTER than ShouldAdvertise in one specific way: it
+// excludes degraded. It is NOT a general "only healthy" predicate — it skips
+// only statuses that positively establish the backend is a bad bet, and admits
+// everything else, including not-yet-classified.
+//
+// The degraded asymmetry is the fix for #5861. Advertising a degraded backend's
+// tools is cheap, but blocking `initialize` on it is not: session creation waits
+// for every backend it attempts (session.makeBaseSession's wg.Wait), so a single
+// slow backend sets the floor for the entire tenant's session-establishment
+// latency. Worse, the handshake makes several sequential round trips, so the
+// cost is a multiple of the backend's per-request latency, not one unit of it.
+//
+// A backend is marked degraded precisely because it is slow — which is exactly
+// the property that must stay off the session-establishment critical path. Its
+// tools remain advertised and callable (ShouldAdvertise still admits it); only
+// the blocking per-session connect is skipped, and the health monitor keeps
+// probing it on its own schedule so recovery is picked up normally.
+//
+// Unknown is admitted, unlike in ShouldAdvertise. The two filters answer
+// different questions and must diverge here. Advertising a tool from a backend
+// of unknown health risks surfacing a capability that cannot be served, so
+// aggregation waits for confirmation. Session establishment has the opposite
+// default: serving is not gated on the first health check completing (only the
+// status reporter calls WaitForInitialHealthChecks), so sessions are routinely
+// created while backends are still Unknown — during pod startup, and for a
+// backend whose first check failed below the unhealthy threshold, which the
+// monitor records as Unknown with a non-zero failure count
+// (health/status.go RecordFailure). Skipping those would connect a session to
+// zero backends during the startup window, which is both a regression against
+// the pre-#5861 behaviour and a worse failure than the one being fixed. "Not yet
+// known to be bad" must therefore fail open.
+func ShouldOpenSession(status vmcp.BackendHealthStatus) bool {
+	// Skip only confirmed-bad statuses; everything else — including Unknown and
+	// the empty zero value — is attempted.
+	return status != vmcp.BackendDegraded &&
+		status != vmcp.BackendUnhealthy &&
+		status != vmcp.BackendUnauthenticated
+}

pkg/vmcp/health/policy_test.go

@@ -0,0 +1,104 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package health
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/stacklok/toolhive/pkg/vmcp"
+)
+
+// TestShouldAdvertiseAndShouldOpenSession pins both health predicates against
+// every BackendHealthStatus constant, plus the empty zero value and an
+// unrecognized value.
+//
+// The two are asserted together because their relationship is the contract that
+// matters, and it is not a simple ordering: ShouldOpenSession is stricter for
+// degraded (advertising a slow backend's tools is cheap, blocking initialize on
+// it is not — #5861) and looser for unknown (session establishment must not fail
+// closed before the first health check completes). Testing them side by side
+// makes an accidental change to either one visible as a change in the pairing.
+//
+// The unrecognized-value row pins a subtlety worth stating explicitly: the two
+// predicates have opposite defaults for a status neither knows about.
+// ShouldAdvertise is an allow-list, so an unrecognized status is NOT advertised
+// (fails closed — a capability that may not be servable is withheld).
+// ShouldOpenSession is a deny-list, so it IS attempted (fails open — better to
+// connect to a backend of uncertain health than to strand a session with none).
+// Each default is the conservative choice for its own question, but they point
+// in opposite directions, so anyone adding a status must consider both.
+func TestShouldAdvertiseAndShouldOpenSession(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name            string
+		status          vmcp.BackendHealthStatus
+		wantAdvertise   bool
+		wantOpenSession bool
+	}{
+		{
+			name:            "empty means health monitoring disabled: assume usable",
+			status:          "",
+			wantAdvertise:   true,
+			wantOpenSession: true,
+		},
+		{
+			name:            "healthy",
+			status:          vmcp.BackendHealthy,
+			wantAdvertise:   true,
+			wantOpenSession: true,
+		},
+		{
+			// The asymmetry that makes #5861's fix work: still advertised, but
+			// never blocked on during session establishment.
+			name:            "degraded is advertisable but not worth blocking initialize on",
+			status:          vmcp.BackendDegraded,
+			wantAdvertise:   true,
+			wantOpenSession: false,
+		},
+		{
+			name:            "unhealthy",
+			status:          vmcp.BackendUnhealthy,
+			wantAdvertise:   false,
+			wantOpenSession: false,
+		},
+		{
+			// The other asymmetry: aggregation waits for confirmation, session
+			// establishment must not, or a cold monitor connects sessions to zero
+			// backends during pod startup.
+			name:            "unknown is not advertised but is still attempted",
+			status:          vmcp.BackendUnknown,
+			wantAdvertise:   false,
+			wantOpenSession: true,
+		},
+		{
+			name:            "unauthenticated (operator misconfiguration)",
+			status:          vmcp.BackendUnauthenticated,
+			wantAdvertise:   false,
+			wantOpenSession: false,
+		},
+		{
+			// ShouldAdvertise is an allow-list (fails closed); ShouldOpenSession is
+			// a deny-list (fails open). Opposite defaults, deliberately — see the
+			// doc comment above.
+			name:            "unrecognized status: not advertised, but still attempted",
+			status:          vmcp.BackendHealthStatus("some-future-status"),
+			wantAdvertise:   false,
+			wantOpenSession: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.Equal(t, tt.wantAdvertise, ShouldAdvertise(tt.status),
+				"ShouldAdvertise(%q)", tt.status)
+			assert.Equal(t, tt.wantOpenSession, ShouldOpenSession(tt.status),
+				"ShouldOpenSession(%q)", tt.status)
+		})
+	}
+}

pkg/vmcp/server/server.go

@@ -506,6 +506,13 @@ func New(
 		OptimizerFactory:  cfg.OptimizerFactory,
 		TelemetryProvider: cfg.TelemetryProvider,
 		AdvertiseFromCore: true,
+		// Gate per-session backend connects on health status (#5861). Without this a
+		// backend the monitor already knows is bad is still re-attempted by every new
+		// session, and session creation blocks on it — so one slow backend sets the
+		// floor for the whole tenant's initialize latency. BackendHealth() returns a
+		// true nil interface when monitoring is disabled, which the session manager
+		// reads as "attempt every backend" (the prior behaviour).
+		BackendHealth: coreVMCP.BackendHealth(),
 	}
 
 	srv, err := Serve(ctx, coreVMCP, deriveServerConfig(resolved, backendRegistry, sessMgrCfg))

pkg/vmcp/server/sessionmanager/factory.go

@@ -22,6 +22,7 @@ import (
 	"github.com/stacklok/toolhive/pkg/telemetry"
 	"github.com/stacklok/toolhive/pkg/vmcp"
 	"github.com/stacklok/toolhive/pkg/vmcp/conversion"
+	"github.com/stacklok/toolhive/pkg/vmcp/health"
 	"github.com/stacklok/toolhive/pkg/vmcp/optimizer"
 	vmcpsession "github.com/stacklok/toolhive/pkg/vmcp/session"
 	"github.com/stacklok/toolhive/pkg/vmcp/session/optimizerdec"
@@ -83,6 +84,14 @@ type FactoryConfig struct {
 	// decorator branch the false case used to select is now unreachable — its
 	// deletion is tracked in #6103.
 	AdvertiseFromCore bool
+
+	// BackendHealth gates which backends a new session attempts to connect to.
+	//
+	// Optional: nil disables health gating and every backend is attempted, which
+	// is both the pre-#5861 behaviour and the correct fallback when health
+	// monitoring is switched off. See health.ShouldOpenSession for why session
+	// establishment applies a stricter predicate than capability advertising.
+	BackendHealth health.StatusProvider
 }
 
 // resolveOptimizer wires the optimizer factory from cfg, applying telemetry

pkg/vmcp/server/sessionmanager/session_manager.go

@@ -27,6 +27,7 @@ import (
 	"github.com/stacklok/toolhive/pkg/cache"
 	transportsession "github.com/stacklok/toolhive/pkg/transport/session"
 	"github.com/stacklok/toolhive/pkg/vmcp"
+	"github.com/stacklok/toolhive/pkg/vmcp/health"
 	"github.com/stacklok/toolhive/pkg/vmcp/optimizer"
 	vmcpsession "github.com/stacklok/toolhive/pkg/vmcp/session"
 	sessiontypes "github.com/stacklok/toolhive/pkg/vmcp/session/types"
@@ -77,6 +78,11 @@ type Manager struct {
 	factory    vmcpsession.MultiSessionFactory
 	backendReg vmcp.BackendRegistry
 
+	// backendHealth gates which backends a new session connects to, or nil when
+	// health monitoring is disabled (every backend is attempted). Read-only here;
+	// the core owns the monitor's lifecycle. See shouldOpenSession.
+	backendHealth health.StatusProvider
+
 	// sessions is a node-local cache of live MultiSession objects, separate
 	// from storage because MultiSession contains un-serialisable runtime state
 	// (HTTP connections, routing tables). On a cache miss it restores the
@@ -132,8 +138,9 @@ func New(
 	// Build the Manager first so we can reference sm.Terminate and sm.sessions
 	// directly in closures, eliminating the forward-reference variable pattern.
 	sm := &Manager{
-		storage:    storage,
-		backendReg: backendRegistry,
+		storage:       storage,
+		backendReg:    backendRegistry,
+		backendHealth: cfg.BackendHealth,
 	}
 
 	// Surface the resolved optimizer factory to the Serve path. The constructor
@@ -835,12 +842,44 @@ func (sm *Manager) DecorateSession(sessionID string, fn func(sessiontypes.MultiS
 	return nil
 }
 
-// listAllBackends returns all backends from the registry as a pointer slice.
+// listAllBackends returns the backends a new session should attempt to connect
+// to, skipping any the health monitor has already classified as not worth
+// blocking on (see shouldOpenSession).
 func (sm *Manager) listAllBackends(ctx context.Context) []*vmcp.Backend {
 	raw := sm.backendReg.List(ctx)
-	backends := make([]*vmcp.Backend, len(raw))
+	backends := make([]*vmcp.Backend, 0, len(raw))
+	skipped := 0
 	for i := range raw {
-		backends[i] = &raw[i]
+		if !sm.shouldOpenSession(&raw[i]) {
+			skipped++
+			continue
+		}
+		backends = append(backends, &raw[i])
+	}
+	if skipped > 0 {
+		//nolint:gosec // G706: values are internal counts, not user-controlled
+		slog.Debug("skipping backends for session establishment due to health status",
+			"skipped", skipped,
+			"attempted", len(backends))
 	}
 	return backends
 }
+
+// shouldOpenSession reports whether this session should attempt to connect to
+// backend, consulting the health monitor when one is wired.
+//
+// A nil provider means health monitoring is disabled: every backend is
+// attempted, preserving the pre-#5861 behaviour. A backend the monitor does not
+// track yet falls back to its registry status. Either way only confirmed-bad
+// statuses are skipped — see health.ShouldOpenSession for why "not yet
+// classified" must fail open rather than closed.
+func (sm *Manager) shouldOpenSession(backend *vmcp.Backend) bool {
+	if sm.backendHealth == nil {
+		return true
+	}
+	status, ok := sm.backendHealth.QueryBackendStatus(backend.ID)
+	if !ok {
+		status = backend.HealthStatus
+	}
+	return health.ShouldOpenSession(status)
+}