diff --git a/pkg/vmcp/core/core_vmcp.go b/pkg/vmcp/core/core_vmcp.go index 98c5523fca..e2102e62c3 100644 --- a/pkg/vmcp/core/core_vmcp.go +++ b/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++ diff --git a/pkg/vmcp/health/policy.go b/pkg/vmcp/health/policy.go new file mode 100644 index 0000000000..28355b3d72 --- /dev/null +++ b/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 +} diff --git a/pkg/vmcp/health/policy_test.go b/pkg/vmcp/health/policy_test.go new file mode 100644 index 0000000000..f3979fb4a1 --- /dev/null +++ b/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) + }) + } +} diff --git a/pkg/vmcp/server/server.go b/pkg/vmcp/server/server.go index 2ec12bc59e..fa094d1b9b 100644 --- a/pkg/vmcp/server/server.go +++ b/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)) diff --git a/pkg/vmcp/server/sessionmanager/factory.go b/pkg/vmcp/server/sessionmanager/factory.go index 73bbc3e9f5..3bb1092d6e 100644 --- a/pkg/vmcp/server/sessionmanager/factory.go +++ b/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 diff --git a/pkg/vmcp/server/sessionmanager/session_manager.go b/pkg/vmcp/server/sessionmanager/session_manager.go index b1808e99ca..ae4fe94532 100644 --- a/pkg/vmcp/server/sessionmanager/session_manager.go +++ b/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) +} diff --git a/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go b/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go new file mode 100644 index 0000000000..fa84660dce --- /dev/null +++ b/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go @@ -0,0 +1,313 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package sessionmanager + +import ( + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + mcpmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" + mcpserver "github.com/stacklok/toolhive-core/mcpcompat/server" + transportsession "github.com/stacklok/toolhive/pkg/transport/session" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/health" + vmcpsession "github.com/stacklok/toolhive/pkg/vmcp/session" +) + +// slowBackendDelay is the per-request delay of the simulated slow-but-working +// backend. It stands in for the real 10-25s latency in #5861, scaled down so +// the suite stays fast while remaining orders of magnitude above the healthy +// backends' sub-millisecond responses. +const slowBackendDelay = 2 * time.Second + +// slowBackendLatencyBudget is the ceiling on total CreateSession latency for a +// tenant containing one known-bad slow backend. It backs the secondary +// (latency) assertion; the primary assertion is the request counter, which is +// deterministic. Set well below slowBackendDelay: if session creation blocks on +// the slow backend at all, elapsed time lands at or above slowBackendDelay. +const slowBackendLatencyBudget = slowBackendDelay / 2 + +// startSlowMCPBackend starts an in-process MCP backend that delays every +// request by delay before responding. It is deliberately WORKING, not broken: +// #5861 is about a backend that is merely slow, which is why simulating it with +// a connection refusal or a 5xx would not reproduce the failure. +// +// The returned counter reports how many HTTP requests the backend received. It +// is the deterministic signal that the backend was (or was not) contacted: +// asserting it is zero proves the skip directly, where a latency assertion only +// infers it and is vulnerable to CI noise. +func startSlowMCPBackend(t *testing.T, backendID string, delay time.Duration) (*vmcp.Backend, *atomic.Int64) { + t.Helper() + mcpSrv := mcpserver.NewMCPServer(backendID, "1.0.0") + mcpSrv.AddTool( + mcpmcp.NewTool("slow_tool", + mcpmcp.WithDescription("A tool on a slow-but-working backend"), + mcpmcp.WithString("input", mcpmcp.Required()), + ), + func(_ context.Context, req mcpmcp.CallToolRequest) (*mcpmcp.CallToolResult, error) { + args, _ := req.Params.Arguments.(map[string]any) + input, _ := args["input"].(string) + return &mcpmcp.CallToolResult{ + Content: []mcpmcp.Content{mcpmcp.NewTextContent(input)}, + }, nil + }, + ) + streamableSrv := mcpserver.NewStreamableHTTPServer(mcpSrv) + var requests atomic.Int64 + mux := http.NewServeMux() + // Count and delay every request, including the initialize handshake — the + // delay is what makes this backend slow rather than broken. + mux.Handle("/mcp", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + time.Sleep(delay) + streamableSrv.ServeHTTP(w, r) + })) + ts := httptest.NewServer(mux) + t.Cleanup(ts.Close) + return &vmcp.Backend{ + ID: backendID, + Name: backendID, + BaseURL: ts.URL + "/mcp", + TransportType: "streamable-http", + }, &requests +} + +// staticHealth is a health.StatusProvider stub returning fixed per-backend +// statuses, standing in for a live health.Monitor that has already classified +// the slow backend. #5861's premise is that the monitor ALREADY knows the +// backend is bad; the bug is that session creation never asks. +type staticHealth map[string]vmcp.BackendHealthStatus + +func (s staticHealth) QueryBackendStatus(backendID string) (vmcp.BackendHealthStatus, bool) { + status, ok := s[backendID] + return status, ok +} + +// newTestManagerWithHealth creates a Manager over backends with the given +// health.StatusProvider (nil = health monitoring disabled). Uses in-memory +// session storage; the health-gating tests do not exercise Redis. +func newTestManagerWithHealth( + t *testing.T, backends []*vmcp.Backend, backendHealth health.StatusProvider, +) *Manager { + t.Helper() + backendList := make([]vmcp.Backend, len(backends)) + for i, b := range backends { + backendList[i] = *b + } + registry := vmcp.NewImmutableRegistry(backendList) + factory := vmcpsession.NewSessionFactory(newUnauthenticatedAuthRegistry(t)) + + storage, err := transportsession.NewLocalSessionDataStorage(time.Hour) + require.NoError(t, err) + t.Cleanup(func() { _ = storage.Close() }) + + sm, cleanup, err := New( + storage, + &FactoryConfig{Base: factory, BackendHealth: backendHealth}, + registry, + ) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, cleanup(context.Background())) }) + return sm +} + +// --------------------------------------------------------------------------- +// Regression tests: #5861 — a slow backend must not inflate tenant session +// latency once the health monitor has classified it +// --------------------------------------------------------------------------- + +// TestRegression_CreateSession_SkipsKnownBadSlowBackend pins the fix for #5861: +// a backend the health monitor has already classified as bad must not be +// re-attempted during per-session backend init. +// +// Before the fix, Manager.listAllBackends returned the registry unfiltered and +// session.makeBaseSession blocked on wg.Wait() for every backend, so a single +// slow backend set the floor for the whole tenant's initialize latency — a +// 10-25s backend against a ~30s client timeout is the reported coin flip. +// Health status gated capability aggregation (core.filterHealthyBackends) but +// never connection establishment, and establishment runs first. +// +// The primary assertion is the slow backend's request counter: zero requests +// proves the skip directly and deterministically. Elapsed time is asserted as a +// secondary signal — it is what the user actually experiences, but on its own it +// is vulnerable to CI noise (slow runners, GC pauses). +// +// The degraded case is the reason the fix cannot simply reuse +// core.filterHealthyBackends unchanged. The reported backend oscillated +// unhealthy -> degraded -> unhealthy continuously, because its 10-25s latency +// straddles the compiled-in 5s degraded threshold and 10s probe timeout +// (health/monitor.go) with no hysteresis. filterHealthyBackends INCLUDES +// degraded, so reusing that predicate verbatim would leave every degraded-phase +// session paying the full latency and the coin flip would survive the fix. See +// health.ShouldOpenSession. +func TestRegression_CreateSession_SkipsKnownBadSlowBackend(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + status vmcp.BackendHealthStatus + }{ + { + // The monitor has classified the backend unhealthy because its real + // latency exceeds the probe timeout — the state the reporter observed. + name: "unhealthy", + status: vmcp.BackendUnhealthy, + }, + { + // The other half of the flap: slow enough to be degraded, not slow + // enough to be unhealthy. + name: "degraded", + status: vmcp.BackendDegraded, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fast := startMCPBackend(t, "backend-fast", "echo") + slow, slowRequests := startSlowMCPBackend(t, "backend-slow", slowBackendDelay) + + sm := newTestManagerWithHealth(t, []*vmcp.Backend{fast, slow}, staticHealth{ + fast.ID: vmcp.BackendHealthy, + slow.ID: tt.status, + }) + + start := time.Now() + sessionID := createSession(t, sm, nil) + elapsed := time.Since(start) + + require.NotEmpty(t, sessionID) + assert.Zero(t, slowRequests.Load(), + "session creation must not contact a backend already known %s "+ + "(#5861); it received %d request(s)", tt.status, slowRequests.Load()) + assert.Less(t, elapsed, slowBackendLatencyBudget, + "session creation must not block on a backend already known %s; "+ + "took %v, budget %v — the slow backend's %v per-request delay is "+ + "on the critical path", tt.status, elapsed, slowBackendLatencyBudget, + slowBackendDelay) + }) + } +} + +// TestRegression_CreateSession_HealthyBackendStillConnected guards against the +// fix over-filtering. Without it, the latency tests above would pass +// vacuously if the fix skipped every backend. +func TestRegression_CreateSession_HealthyBackendStillConnected(t *testing.T) { + t.Parallel() + + fast := startMCPBackend(t, "backend-fast", "echo") + sm := newTestManagerWithHealth(t, []*vmcp.Backend{fast}, staticHealth{ + fast.ID: vmcp.BackendHealthy, + }) + + sessionID := createSession(t, sm, nil) + + sess, ok := sm.GetMultiSession(context.Background(), sessionID) + require.True(t, ok) + require.NotNil(t, sess) + assert.NotEmpty(t, sess.BackendSessions(), + "a healthy backend must still hold a session — the health filter must "+ + "not drop everything") +} + +// TestRegression_CreateSession_UnknownStatusIsAttempted pins that "not yet +// classified" never fails closed. #5861's fix must skip backends known to be +// bad, not backends nothing is known about yet. +// +// Two distinct paths produce Unknown, and both must still be attempted: +// +// - Untracked (exists=false): the monitor has not created state for the +// backend yet. The registry status is consulted, and the k8s workload +// mapper returns BackendUnknown for a Pending phase +// (workloads/k8s.go mapK8SWorkloadPhaseToHealth). +// - Tracked as Unknown (exists=true): the monitor recorded a first failure +// BELOW the unhealthy threshold, which stores status Unknown with a +// non-zero failure count (health/status.go RecordFailure). Nothing is +// confirmed yet — the default threshold needs 3 consecutive failures. +// +// Serving is not gated on the first health check completing (only the status +// reporter calls WaitForInitialHealthChecks), so sessions are created while +// backends are still Unknown. Failing closed there would connect a session to +// zero backends during the startup window — a regression against pre-#5861 +// behaviour and a worse outcome than the bug being fixed. +func TestRegression_CreateSession_UnknownStatusIsAttempted(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // monitored is the status the health monitor reports, or "" for a + // backend the monitor does not track at all (exists=false). + monitored vmcp.BackendHealthStatus + tracked bool + // registryStatus is the status the backend carries in the registry. + registryStatus vmcp.BackendHealthStatus + }{ + { + name: "untracked by monitor, registry reports unknown (k8s Pending)", + tracked: false, + registryStatus: vmcp.BackendUnknown, + }, + { + name: "untracked by monitor, registry status unset", + tracked: false, + registryStatus: "", + }, + { + name: "tracked as unknown (first failure below unhealthy threshold)", + monitored: vmcp.BackendUnknown, + tracked: true, + registryStatus: vmcp.BackendHealthy, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + backend := startMCPBackend(t, "backend-pending", "echo") + backend.HealthStatus = tt.registryStatus + + health := staticHealth{} + if tt.tracked { + health[backend.ID] = tt.monitored + } + + sm := newTestManagerWithHealth(t, []*vmcp.Backend{backend}, health) + sessionID := createSession(t, sm, nil) + + sess, ok := sm.GetMultiSession(context.Background(), sessionID) + require.True(t, ok) + require.NotNil(t, sess) + assert.NotEmpty(t, sess.BackendSessions(), + "a backend whose health is not yet established must still be "+ + "attempted — session establishment must not fail closed before "+ + "the health monitor has confirmed anything") + }) + } +} + +// TestRegression_CreateSession_NilHealthProviderConnectsAll pins that disabling +// health monitoring preserves the pre-fix behaviour exactly: every backend is +// attempted. A nil provider must not be read as "everything is unhealthy". +func TestRegression_CreateSession_NilHealthProviderConnectsAll(t *testing.T) { + t.Parallel() + + fast := startMCPBackend(t, "backend-fast", "echo") + sm := newTestManagerWithHealth(t, []*vmcp.Backend{fast}, nil) + + sessionID := createSession(t, sm, nil) + + sess, ok := sm.GetMultiSession(context.Background(), sessionID) + require.True(t, ok) + assert.NotEmpty(t, sess.BackendSessions(), + "with health monitoring disabled every backend must be attempted") +}