From 73108de93ea5389e59db25789684f5d5bc99bf7b Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Fri, 31 Jul 2026 10:08:12 -0700 Subject: [PATCH 1/3] Skip known-bad backends when opening sessions A backend that is merely slow (not down) could drag an entire VirtualMCPServer tenant's client-facing initialize success rate to roughly a coin flip, with no self-healing. Health status gated capability aggregation but never connection establishment, and establishment runs first: listAllBackends returned the registry unfiltered and makeBaseSession blocks on every backend it attempts. One slow backend therefore set the floor for the whole tenant's session latency. Because the handshake makes several sequential round trips, the cost is a multiple of the backend's per-request latency -- a 2s backend delays session creation by 8s -- so a 10-25s backend loses the race against a typical ~30s client timeout. Session establishment now consults the health monitor and skips backends already known bad. The predicate is deliberately stricter than the advertising one: it excludes degraded, because degraded means slow, which is precisely what must stay off the critical path. A degraded backend's tools remain advertised and callable, and the monitor keeps probing it, so recovery is unaffected. Without this asymmetry the fix would not hold: the reported backend oscillated unhealthy <-> degraded continuously, so reusing the aggregation predicate would leave every degraded-phase session paying full latency. Health monitoring being disabled preserves prior behaviour -- every backend is attempted. --- pkg/vmcp/core/core_vmcp.go | 6 +- pkg/vmcp/health/policy.go | 43 ++++ pkg/vmcp/server/server.go | 7 + pkg/vmcp/server/sessionmanager/factory.go | 9 + .../server/sessionmanager/session_manager.go | 48 +++- .../slow_backend_regression_test.go | 227 ++++++++++++++++++ 6 files changed, 332 insertions(+), 8 deletions(-) create mode 100644 pkg/vmcp/health/policy.go create mode 100644 pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go 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..ec53704738 --- /dev/null +++ b/pkg/vmcp/health/policy.go @@ -0,0 +1,43 @@ +// 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: it excludes degraded. +// +// The 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. +func ShouldOpenSession(status vmcp.BackendHealthStatus) bool { + return status == "" || status == vmcp.BackendHealthy +} 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..d787072c67 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,43 @@ 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 (not yet probed) falls back to its registry status, so a cold monitor at +// startup does not block every session. +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..e12f26f90a --- /dev/null +++ b/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go @@ -0,0 +1,227 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package sessionmanager + +import ( + "context" + "net/http" + "net/http/httptest" + "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. Set well below slowBackendDelay: +// if session creation blocks on the slow backend at all, elapsed time lands at +// or above slowBackendDelay and the assertion fails. +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. +func startSlowMCPBackend(t *testing.T, backendID string, delay time.Duration) *vmcp.Backend { + 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) + mux := http.NewServeMux() + // Delay every request, including the initialize handshake — that is what + // makes this backend slow rather than broken. + mux.Handle("/mcp", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + 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", + } +} + +// 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_SkipsKnownUnhealthySlowBackend pins the fix for +// #5861: a backend the health monitor has already marked unhealthy 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 assertion is on elapsed time rather than on the session's backend set, +// because the user-visible symptom is latency: a fix that kept the backend in +// the set but stopped blocking on it would also be correct, while a fix that +// dropped it for an unrelated reason would not be. +func TestRegression_CreateSession_SkipsKnownUnhealthySlowBackend(t *testing.T) { + t.Parallel() + + fast := startMCPBackend(t, "backend-fast", "echo") + slow := startSlowMCPBackend(t, "backend-slow", slowBackendDelay) + + // The health monitor has already classified the slow backend as unhealthy + // (its real latency exceeds the probe timeout). This is the state the + // reporter observed; the bug is that CreateSession ignores it. + sm := newTestManagerWithHealth(t, []*vmcp.Backend{fast, slow}, staticHealth{ + fast.ID: vmcp.BackendHealthy, + slow.ID: vmcp.BackendUnhealthy, + }) + + start := time.Now() + sessionID := createSession(t, sm, nil) + elapsed := time.Since(start) + + require.NotEmpty(t, sessionID) + assert.Less(t, elapsed, slowBackendLatencyBudget, + "session creation must not block on a backend already known unhealthy "+ + "(#5861); took %v, budget %v — the slow backend's %v delay is on the "+ + "critical path", elapsed, slowBackendLatencyBudget, slowBackendDelay) +} + +// TestRegression_CreateSession_DegradedSlowBackendDoesNotBlockTenant is the +// other half of #5861 and the reason the fix cannot simply reuse +// core.filterHealthyBackends unchanged. +// +// The reported backend oscillated unhealthy -> degraded -> unhealthy forever, +// 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 — the coin +// flip would survive the fix. Session establishment therefore uses a stricter +// predicate than aggregation: degraded is advertisable but not worth blocking +// initialize on. See health.ShouldOpenSession. +func TestRegression_CreateSession_DegradedSlowBackendDoesNotBlockTenant(t *testing.T) { + t.Parallel() + + fast := startMCPBackend(t, "backend-fast", "echo") + slow := startSlowMCPBackend(t, "backend-slow", slowBackendDelay) + + sm := newTestManagerWithHealth(t, []*vmcp.Backend{fast, slow}, staticHealth{ + fast.ID: vmcp.BackendHealthy, + slow.ID: vmcp.BackendDegraded, + }) + + start := time.Now() + sessionID := createSession(t, sm, nil) + elapsed := time.Since(start) + + require.NotEmpty(t, sessionID) + assert.Less(t, elapsed, slowBackendLatencyBudget, + "a degraded (slow-but-working) backend must not block session creation "+ + "either (#5861 flapping); took %v, budget %v", elapsed, slowBackendLatencyBudget) +} + +// TestRegression_CreateSession_HealthyBackendStillConnected guards against the +// fix over-filtering. Without it, the two 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_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") +} From e8f50bd2725f97eb209940a6572b98717b773c33 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Fri, 31 Jul 2026 10:48:31 -0700 Subject: [PATCH 2/3] Fail open when backend health is not yet known Address review feedback on the session-establishment health gate. The gate skipped any status that was not healthy, which included Unknown and so failed closed before the monitor had confirmed anything. Two paths reach it: a backend the monitor does not track yet (a Pending k8s workload maps to Unknown), and one whose first check failed below the unhealthy threshold, which is recorded as Unknown with a non-zero failure count. Serving is not gated on the first health check, so sessions are created in that window and would have connected to zero backends -- a regression against the prior behaviour and worse than the bug being fixed. The gate now skips only confirmed-bad statuses. The regression tests asserted the skip via wall-clock latency alone, which is vulnerable to CI noise. The slow backend now counts the requests it receives, so the skip is asserted directly; the latency bound is kept as a secondary signal of what the user experiences. Verified both assertions still fail without the fix (4 requests, 8s) -- the counter also shows the handshake makes four sequential round trips, which is why a 2s backend costs 8s. The two latency tests differed only by status, so they are now one table-driven test. --- pkg/vmcp/health/policy.go | 31 ++- .../server/sessionmanager/session_manager.go | 5 +- .../slow_backend_regression_test.go | 212 ++++++++++++------ 3 files changed, 178 insertions(+), 70 deletions(-) diff --git a/pkg/vmcp/health/policy.go b/pkg/vmcp/health/policy.go index ec53704738..28355b3d72 100644 --- a/pkg/vmcp/health/policy.go +++ b/pkg/vmcp/health/policy.go @@ -24,11 +24,14 @@ func ShouldAdvertise(status vmcp.BackendHealthStatus) bool { // ShouldOpenSession reports whether a new session should attempt to open a // connection to a backend in this status. // -// This is deliberately STRICTER than ShouldAdvertise: it excludes degraded. +// 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 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 +// 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. @@ -38,6 +41,24 @@ func ShouldAdvertise(status vmcp.BackendHealthStatus) bool { // 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 { - return status == "" || status == vmcp.BackendHealthy + // 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/server/sessionmanager/session_manager.go b/pkg/vmcp/server/sessionmanager/session_manager.go index d787072c67..ae4fe94532 100644 --- a/pkg/vmcp/server/sessionmanager/session_manager.go +++ b/pkg/vmcp/server/sessionmanager/session_manager.go @@ -870,8 +870,9 @@ func (sm *Manager) listAllBackends(ctx context.Context) []*vmcp.Backend { // // A nil provider means health monitoring is disabled: every backend is // attempted, preserving the pre-#5861 behaviour. A backend the monitor does not -// track (not yet probed) falls back to its registry status, so a cold monitor at -// startup does not block every session. +// 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 diff --git a/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go b/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go index e12f26f90a..fa84660dce 100644 --- a/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go +++ b/pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go @@ -7,6 +7,7 @@ import ( "context" "net/http" "net/http/httptest" + "sync/atomic" "testing" "time" @@ -28,16 +29,22 @@ import ( const slowBackendDelay = 2 * time.Second // slowBackendLatencyBudget is the ceiling on total CreateSession latency for a -// tenant containing one known-bad slow backend. Set well below slowBackendDelay: -// if session creation blocks on the slow backend at all, elapsed time lands at -// or above slowBackendDelay and the assertion fails. +// 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. -func startSlowMCPBackend(t *testing.T, backendID string, delay time.Duration) *vmcp.Backend { +// +// 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( @@ -54,10 +61,12 @@ func startSlowMCPBackend(t *testing.T, backendID string, delay time.Duration) *v }, ) streamableSrv := mcpserver.NewStreamableHTTPServer(mcpSrv) + var requests atomic.Int64 mux := http.NewServeMux() - // Delay every request, including the initialize handshake — that is what - // makes this backend slow rather than broken. + // 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) })) @@ -68,7 +77,7 @@ func startSlowMCPBackend(t *testing.T, backendID string, delay time.Duration) *v Name: backendID, BaseURL: ts.URL + "/mcp", TransportType: "streamable-http", - } + }, &requests } // staticHealth is a health.StatusProvider stub returning fixed per-backend @@ -115,8 +124,8 @@ func newTestManagerWithHealth( // latency once the health monitor has classified it // --------------------------------------------------------------------------- -// TestRegression_CreateSession_SkipsKnownUnhealthySlowBackend pins the fix for -// #5861: a backend the health monitor has already marked unhealthy must not be +// 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 @@ -126,70 +135,71 @@ func newTestManagerWithHealth( // Health status gated capability aggregation (core.filterHealthyBackends) but // never connection establishment, and establishment runs first. // -// The assertion is on elapsed time rather than on the session's backend set, -// because the user-visible symptom is latency: a fix that kept the backend in -// the set but stopped blocking on it would also be correct, while a fix that -// dropped it for an unrelated reason would not be. -func TestRegression_CreateSession_SkipsKnownUnhealthySlowBackend(t *testing.T) { +// 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() - fast := startMCPBackend(t, "backend-fast", "echo") - slow := startSlowMCPBackend(t, "backend-slow", slowBackendDelay) - - // The health monitor has already classified the slow backend as unhealthy - // (its real latency exceeds the probe timeout). This is the state the - // reporter observed; the bug is that CreateSession ignores it. - sm := newTestManagerWithHealth(t, []*vmcp.Backend{fast, slow}, staticHealth{ - fast.ID: vmcp.BackendHealthy, - slow.ID: vmcp.BackendUnhealthy, - }) - - start := time.Now() - sessionID := createSession(t, sm, nil) - elapsed := time.Since(start) - - require.NotEmpty(t, sessionID) - assert.Less(t, elapsed, slowBackendLatencyBudget, - "session creation must not block on a backend already known unhealthy "+ - "(#5861); took %v, budget %v — the slow backend's %v delay is on the "+ - "critical path", elapsed, slowBackendLatencyBudget, slowBackendDelay) -} + 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, + }, + } -// TestRegression_CreateSession_DegradedSlowBackendDoesNotBlockTenant is the -// other half of #5861 and the reason the fix cannot simply reuse -// core.filterHealthyBackends unchanged. -// -// The reported backend oscillated unhealthy -> degraded -> unhealthy forever, -// 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 — the coin -// flip would survive the fix. Session establishment therefore uses a stricter -// predicate than aggregation: degraded is advertisable but not worth blocking -// initialize on. See health.ShouldOpenSession. -func TestRegression_CreateSession_DegradedSlowBackendDoesNotBlockTenant(t *testing.T) { - t.Parallel() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() - fast := startMCPBackend(t, "backend-fast", "echo") - slow := startSlowMCPBackend(t, "backend-slow", slowBackendDelay) + 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: vmcp.BackendDegraded, - }) + 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) + start := time.Now() + sessionID := createSession(t, sm, nil) + elapsed := time.Since(start) - require.NotEmpty(t, sessionID) - assert.Less(t, elapsed, slowBackendLatencyBudget, - "a degraded (slow-but-working) backend must not block session creation "+ - "either (#5861 flapping); took %v, budget %v", elapsed, slowBackendLatencyBudget) + 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 two latency tests above would pass +// 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() @@ -209,6 +219,82 @@ func TestRegression_CreateSession_HealthyBackendStillConnected(t *testing.T) { "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". From edf59da5c2f34d7a057ab82d639ba0036f2200b0 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Fri, 31 Jul 2026 11:39:07 -0700 Subject: [PATCH 3/3] Pin the two health predicates against every status Self-review found the new predicates were only exercised indirectly, via the core aggregation tests and the sessionmanager integration tests. Nothing asserted them directly, and nothing covered a status value that is none of the declared constants. Assert both together, since their relationship is the contract that matters and it is not a simple ordering: ShouldOpenSession is stricter for degraded and looser for unknown. Testing them side by side makes an accidental change to either visible as a change in the pairing. The unrecognized-status row documents that the two have opposite defaults -- ShouldAdvertise is an allow-list and fails closed, ShouldOpenSession is a deny-list and fails open. Each is the conservative choice for its own question, but they point in opposite directions, so a status added later must be considered against both. --- pkg/vmcp/health/policy_test.go | 104 +++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 pkg/vmcp/health/policy_test.go 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) + }) + } +}