Skip known-bad backends when opening vMCP sessions

pkg/vmcp/core/core_vmcp.go

@@ -836,9 +836,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,78 @@
+// 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 session should attempt to open a
+// connection to a backend in this status.
+//
+// It skips only statuses that positively establish the backend is a bad bet and
+// admits everything else, including not-yet-classified. This is the fix for
+// #5861: session establishment waits for every backend it attempts
+// (session.makeBaseSession's wg.Wait), so a backend the monitor already knows is
+// bad sets the floor for the whole 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. The reported
+// backend's 10-25s latency exceeds the 10s probe timeout, so its checks fail and
+// it reaches Unhealthy after UnhealthyThreshold consecutive failures — which is
+// the status this predicate skips.
+//
+// Degraded is deliberately ADMITTED. The tempting reading — "degraded means slow,
+// so don't block on it" — does not survive the status's three producers
+// (see vmcp.BackendDegraded):
+//
+//   - Slow probe (healthChecker.CheckHealth): a successful check slower than
+//     DegradedThreshold. Genuinely slow, and the only producer where skipping
+//     would buy latency.
+//   - Recovering (statusTracker.RecordSuccess): ANY success recorded while
+//     consecutiveFailures > 0 is forced to degraded, overriding the check's own
+//     verdict. The backend just answered, possibly in microseconds, and stays
+//     labelled degraded for up to one CheckInterval (30s default).
+//   - Auth retrying (workloads.mapWorkloadStatusToVMCPHealth): a transient
+//     OAuth-refresh failure, with no latency component at all.
+//
+// Skipping degraded would exclude a fast, working, just-recovered backend from
+// every session created in the ~30s after it recovers, making recovery slower to
+// take effect — a worse and more surprising failure than the one #5861 reports.
+// Telling the producers apart needs a degradation reason the enum does not carry;
+// until it does, the safe reading of degraded at session-open time is "still
+// worth attempting".
+//
+// The residual cost is accepted knowingly: a backend slow enough to be degraded
+// but faster than the probe timeout (DegradedThreshold..Timeout) stays on the
+// session-establishment path. That window is bounded by the probe timeout, where
+// #5861's backend was not.
+//
+// Unknown is admitted for a separate reason: 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 (statusTracker.RecordFailure). Skipping those would
+// connect a session to zero backends during the startup window: both a
+// regression against the pre-#5861 behaviour and a worse failure than the one
+// being fixed. "Not yet known to be bad" must fail open.
+func ShouldOpenSession(status vmcp.BackendHealthStatus) bool {
+	// Skip only confirmed-bad statuses; everything else — including Degraded,
+	// Unknown, and the empty zero value — is attempted.
+	return status != vmcp.BackendUnhealthy &&
+		status != vmcp.BackendUnauthenticated
+}

pkg/vmcp/health/policy_test.go

@@ -0,0 +1,108 @@
+// 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 LOOSER for
+// unknown (session establishment must not fail closed before the first health
+// check completes), and the two now COINCIDE on degraded — the deliberate outcome
+// of #5861's review: that status conflates "slow", "recovering" and "auth
+// retrying", so it cannot be read as "slow" at session-open time. 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,
+		},
+		{
+			// Degraded is attempted, not skipped. Only one of its three producers
+			// is latency; the "recovering" producer forces degraded onto a backend
+			// that just answered successfully, so skipping it would sideline a
+			// fast, working backend for up to one check interval.
+			name:            "degraded is advertised AND attempted",
+			status:          vmcp.BackendDegraded,
+			wantAdvertise:   true,
+			wantOpenSession: true,
+		},
+		{
+			name:            "unhealthy",
+			status:          vmcp.BackendUnhealthy,
+			wantAdvertise:   false,
+			wantOpenSession: false,
+		},
+		{
+			// The asymmetry that remains: 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,21 @@ func New(
 		OptimizerFactory:  cfg.OptimizerFactory,
 		TelemetryProvider: cfg.TelemetryProvider,
 		AdvertiseFromCore: true,
+		// Gate new-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 bad 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).
+		//
+		// A skipped backend's tools stay advertised and callable, but only because
+		// AdvertiseFromCore is set just above: tools/call then routes through
+		// core.CallTool over the core's aggregated view rather than the session's own
+		// routing table, which is built solely from backends that connected. The
+		// session does lose list_changed propagation for a skipped backend, partly
+		// offset by InvalidateCapabilityCache being global (serve_list_changed.go) —
+		// so any other backend's notification opportunistically sweeps in its changes.
+		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,21 @@ 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.
+	// Restored sessions are never filtered (see Manager.listAllBackends).
+	//
+	// 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 which statuses
+	// are skipped.
+	//
+	// Because nil is indistinguishable from "monitoring disabled", an embedder that
+	// calls Serve with a hand-built FactoryConfig and omits this field gets no
+	// health gating and no warning. server.New always populates it from the core's
+	// monitor, so in-tree compositions are covered; direct-Serve callers that want
+	// the #5861 fix must pass it explicitly.
+	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
@@ -329,8 +336,9 @@ func (sm *Manager) CreateSession(
 	// Resolve the caller identity (may be nil for anonymous access).
 	identity, _ := auth.IdentityFromContext(ctx)
 
-	// List all available backends from the registry.
-	backends := sm.listAllBackends(ctx)
+	// List the backends worth opening connections to, skipping any the health
+	// monitor already knows are bad (#5861).
+	backends := sm.listBackendsForNewSession(ctx)
 
 	// Build the fully-formed MultiSession using the SDK-assigned session ID.
 	sess, err := sm.factory.MakeSessionWithID(ctx, sessionID, identity, backends, sink)
@@ -835,7 +843,16 @@ 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 every backend in the registry as a pointer slice,
+// with no health filtering.
+//
+// Used by the restore path, which must offer the factory the full registry: it
+// intersects this list with the session's stored backend IDs
+// (session.RestoreSession), so filtering here would silently DROP a backend the
+// session already held rather than merely defer connecting to one. Because the
+// routing table is rebuilt from whatever reconnects, a dropped backend stays gone
+// for the rest of that session's life even after it recovers. New sessions use
+// listBackendsForNewSession instead.
 func (sm *Manager) listAllBackends(ctx context.Context) []*vmcp.Backend {
 	raw := sm.backendReg.List(ctx)
 	backends := make([]*vmcp.Backend, len(raw))
@@ -844,3 +861,48 @@ func (sm *Manager) listAllBackends(ctx context.Context) []*vmcp.Backend {
 	}
 	return backends
 }
+
+// listBackendsForNewSession returns the backends a NEW session should attempt to
+// connect to, skipping any the health monitor already knows are bad
+// (see shouldOpenSession).
+//
+// Deliberately not used on the restore path — see listAllBackends.
+func (sm *Manager) listBackendsForNewSession(ctx context.Context) []*vmcp.Backend {
+	raw := sm.backendReg.List(ctx)
+	backends := make([]*vmcp.Backend, 0, len(raw))
+	skipped := 0
+	for i := range raw {
+		if !sm.shouldOpenSession(&raw[i]) {
+			skipped++
+			continue
+		}
+		backends = append(backends, &raw[i])
+	}
+	if skipped > 0 {
+		slog.Debug("skipping backends for session establishment due to health status",
+			"skipped", skipped,
+			"attempted", len(backends))
+	}
+	return backends
+}
+
+// shouldOpenSession reports whether a new 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, which is how a status the registry
+// knows but the monitor has not caught up with (e.g. a k8s workload already
+// reported unhealthy) is still honoured. 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)
+}