Skip known-bad backends when opening vMCP sessions - #6162
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6162 +/- ##
==========================================
+ Coverage 72.56% 72.59% +0.02%
==========================================
Files 736 737 +1
Lines 76331 76382 +51
==========================================
+ Hits 55391 55446 +55
+ Misses 17019 17013 -6
- Partials 3921 3923 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses vMCP session-creation latency amplification by skipping backends that the health monitor has already classified as not worth blocking initialize on, instead of re-attempting every backend connection during each new session. It also centralizes the “advertise vs. connect” health policy into shared predicates to avoid duplicated, drifting logic.
Changes:
- Introduces shared health predicates (
health.ShouldAdvertise/health.ShouldOpenSession) and reuses them from core capability aggregation. - Wires an (optional) health status provider into the session manager and filters backends before session establishment.
- Adds regression tests reproducing #5861 with slow-but-working backends.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/health/policy.go | Adds shared health predicates that encode “advertise vs connect” asymmetry. |
| pkg/vmcp/core/core_vmcp.go | Reuses the shared advertise predicate for capability aggregation filtering. |
| pkg/vmcp/server/sessionmanager/factory.go | Extends session manager factory config to accept an optional backend health provider. |
| pkg/vmcp/server/sessionmanager/session_manager.go | Filters backends during session establishment based on health monitor status. |
| pkg/vmcp/server/server.go | Wires core’s backend health provider into the session manager config. |
| pkg/vmcp/server/sessionmanager/slow_backend_regression_test.go | Adds regression tests for slow backend behavior in session creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
github-actions
Bot
added
size/M
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/vmcp/server/sessionmanager/session_manager.go:847
- listAllBackends is used for both CreateSession and RestoreSession (via loadSession), but the doc comment currently says "a new session". Tightening the wording to "session establishment" avoids misleading readers about the restore path also being filtered.
// 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).
pkg/vmcp/health/policy.go:14
- The comment implies an empty health status means monitoring is disabled, but the code is really treating the empty/zero value as "assume healthy" for backward compatibility (which can also cover an unset registry status). Clarifying this avoids readers inferring stronger semantics than the predicate enforces.
// 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.
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.
Self-review findingsRan two independent adversarial reviews against this branch (fresh context, no shared framing) to try to break it. Both cleared the two highest-risk areas; one found a real residual gap I'm disclosing below. One test gap fixed in edf59da. Cleared — the two things most likely to have made this worseSession restore is gated too, not just session creation. My highest-suspicion area was No advertised-but-uncallable trap. Also confirmed: the Fixed:
|
github-actions
Bot
added
size/M
Summary
A backend that is merely slow — not down, not erroring — could drag an entire
VirtualMCPServertenant's client-facinginitializesuccess rate to roughly a coin flip, with no self-healing. The reporter saw this from one backend whose real round-trip latency hovered in the 10-25s range while the tenant's other 18 backends aggregated in ~1s.Root cause: health status gated capability aggregation but never connection establishment, and establishment runs first.
Manager.listAllBackendsreturned the registry unfiltered, andsession.makeBaseSessionblocks onwg.Wait()for every backend it attempts — so one slow backend set the floor for the whole tenant's session latency. The circuit breaker'sCanAttempt()had exactly one production caller: the monitor's own background loop. Every new session re-attempted a backend the monitor already knew was bad and re-paid close to the full timeout.Worse than the issue describes: the handshake makes several sequential round trips, each paying full latency. The regression test measures 8s of session-creation delay from a 2s backend — 4× amplification. So a 10-25s backend costs ~40-100s, which is why the reporter saw hard timeouts rather than merely slow sessions.
What changed:
health.ShouldOpenSession/health.ShouldAdvertise— the health predicate, now shared instead of copied.core.filterHealthyBackendscallsShouldAdvertise(behaviour-preserving).sessionmanagerconsults the health monitor and skips backends already known bad before opening connections.server.NewwirescoreVMCP.BackendHealth()into the session manager config.Fixes #5861
Type of change
Test plan
task test)task test-e2e)task lint-fix)task testpasses in full.task lint-fixreports 4 issues, all pre-existing on a cleanmain(verified viagit stash -u) and none in files this PR touches.The four new regression tests were written before the fix and confirmed to reproduce the failure:
After the fix all four pass (8.01s → 0.00s). The two guard tests pass in both runs by design — they prove the fix is not simply skipping every backend, which would make the latency assertions pass vacuously.
Changes
pkg/vmcp/health/policy.goShouldAdvertise/ShouldOpenSession— the shared health predicate and the rationale for their asymmetry.pkg/vmcp/core/core_vmcp.gofilterHealthyBackendsdelegates tohealth.ShouldAdvertise. No behaviour change.pkg/vmcp/server/sessionmanager/factory.goFactoryConfig.BackendHealth(optional; nil = health gating disabled).pkg/vmcp/server/sessionmanager/session_manager.golistAllBackendsgates via newshouldOpenSession.pkg/vmcp/server/server.gocoreVMCP.BackendHealth()intosessMgrCfg.pkg/vmcp/server/sessionmanager/slow_backend_regression_test.goDoes this introduce a user-facing change?
Yes. A single slow backend no longer inflates
initializelatency for an entire tenant. Once the health monitor marks a backend unhealthy or degraded, new sessions stop blocking on it; its tools stay advertised and callable, and the monitor keeps probing it, so recovery is unaffected. Deployments with health monitoring disabled are unchanged.Special notes for reviewers
The load-bearing decision is the asymmetry.
ShouldOpenSessionexcludesdegraded;ShouldAdvertiseincludes it. Without that, this PR would not close #5861: the reported backend oscillatedunhealthy → degraded → unhealthycontinuously (its 10-25s latency straddles the compiled-in 5s degraded threshold and 10s probe timeout, with no hysteresis), so reusing the aggregation predicate verbatim would leave every degraded-phase session paying full latency.TestRegression_CreateSession_DegradedSlowBackendDoesNotBlockTenantpins this. Worth confirming you agree "degraded = advertisable but not worth blockinginitializeon" is the right policy.Please scrutinise the wiring — it is in core request-path code. Three things I verified rather than assumed:
pkg/vmcp/healthdepends only onvmcp,vmcp/auth/types, andhealth/context.ratelimitandcodemodeboth embedcore.VMCP, soBackendHealth()promotes to the real core through the decorator chain.BackendHealth()returns a true nil interface when monitoring is disabled (core_vmcp.go:260), avoiding the typed-nil-in-interface trap thatserve.go:450documents having previously caused a/readyzpanic.Known limitation: the latency assertions are wall-clock. The 4× amplification gives an 8:1 margin against the 1s budget, which should hold, but flag it if you would rather not have timing assertions in CI.
Not fixed here, deliberately (each its own PR):
operational.timeoutsis inert — validated (config/validator.go:416), defaulted, in the CRD, documented, but never read by production code. The reporter considered raisingtimeouts.perWorkloadas a workaround; it would have done nothing.session.WithBackendInitTimeoutis the ready seam and has no production caller.partialFailureModeis inert — same pattern (validator.go:466).docs/operator/virtualmcpserver-kubernetes-guide.md:562recommendsbest_effortfor exactly this failure mode. Aggregation is hardcoded best-effort atdefault_aggregator.go:173regardless of the setting. Either implement or remove.health/monitor.go:216) — the backend still flaps, it just no longer drags the tenant down while doing so. Design question worth its own issue.Prior art: #3530 attempted the routing half of this and was closed unmerged for PR size (>1000 lines), with a review objection to adding complexity to
defaultAggregator. This PR is ~90 lines of non-test change and touches no aggregator logic beyond delegating an existing predicate.Generated with Claude Code