feat(vmcp): stop session init stalling on backends that never service the notification stream - #6633
feat(vmcp): stop session init stalling on backends that never service the notification stream#6633aron-muon wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6633 +/- ##
==========================================
- Coverage 78.98% 78.92% -0.06%
==========================================
Files 782 782
Lines 78065 78099 +34
==========================================
- Hits 61658 61639 -19
- Misses 16402 16455 +53
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
02f0a03 to
398a79f
Compare
|
Heads up on the two red checks here, they aren't from this branch.
That reproduces on a clean checkout of Put the one-line fix up as #6636. Once that lands I'll rebase and these should go green. |
jhrozek
left a comment
There was a problem hiding this comment.
Automated review pass over the list_changed exclusion + authoritative backendInit timeout changes. Design and overall test coverage look solid; two items below, one of which (the data race) will fail task test under -race as currently written.
398a79f to
4fbe187
Compare
|
Thanks, both fixed, and rebased onto current main now that #6636 has landed. Data race - you're right, and it fails exactly as you describe. My mistake: I ran Went with a mutex-guarded recorder rather than I've since run the whole of Misplaced GoDoc - also correct, and a self-inflicted one: I anchored the insertion on the One thing worth flagging that isn't from this branch: |
|
The one remaining red check here ( It's a pre-existing I checked before blaming main - it reproduces on a pristine checkout at 2bb9996, 5 failures, deterministic: Fix is up as #6644. With it the same filter is 0 failures and |
WithBackendInitTimeout existed but nothing outside tests called it, so session init was pinned to the 30s default. operational.timeouts only ever raised it. Add operational.timeouts.backendInit and wire it, and make an explicitly configured value authoritative so a longer workload request timeout no longer extends it. A backend that stalls the handshake instead of answering or failing now has a bound short enough to lose the race against a client's own connect timeout, which partialFailureMode: best_effort turns into a usable session. Default behaviour is unchanged when backendInit is unset. (cherry picked from commit 7862764)
Serve is too big to unit test, so the two lines wiring backendInit went uncovered. Pull the option building into sessionFactoryOptions and test its branches directly. No behaviour change. (cherry picked from commit 02f0a03)
Subscribing to a backend's list_changed notifications opens a standalone notification stream during session initialization. A backend that accepts the subscribe and then never services it stalls the handshake until the init deadline, and clients with their own connect timeout give up first. The connector already treats a nil sink as "do not subscribe" and its comment says the stream must stay opt-in because some backends hang on it, but the server supplies a sink for every session, so in production that gate was unreachable. Add operational.listChanged, and a WithListChangedFilter factory option in the per-workload resolver shape the factory already uses, so a single misbehaving backend can be excluded while the rest keep live propagation. An excluded backend loses only that: its tools are still aggregated and callable, and they refresh on the next session. disabledWorkloads carries listType=set for the SSA tag the CRD schema checker requires, matching MCPGroup.Servers. Default is unchanged. With no config every backend is subscribed exactly as before.
Two things from @jhrozek's pass: - the sink recorder was written from every initOneBackend goroutine with no synchronisation, so -race failed the test. Guard it with a mutex and read through a snapshot. Mirrors factory_dialcontrol_test.go, which already synchronises its concurrent-connector assertions. - inserting listChangedEnabled put it under isKnownModern's doc comment, leaving one function documented by the other's comment and the other with none. Each has its own again.
4fbe187 to
8e455a9
Compare
|
Correction on the It was my linter, not the tree. I was running a The stale binary doesn't even load the config on current main any more - Meanwhile I've rebased this onto current main (7e52ab9, so Go 1.27) and re-verified with correctly built tooling: The |
|
Two leftovers from the Go 1.27 bump: the job's So the three red checks here are all main's, with fixes queued: #6644 ( |
Summary
Background: vMCP opens a persistent connection to every backend during session
initialize, and part of that is subscribing to the backend's list_changed notifications. A backend that accepts the subscribe and then never services it stalls the handshake, and clients with their own connect timeout give up before it resolves. Two separate things make that unrecoverable rather than merely slow:createMCPClientonly setsWithContinuousListening()when the sink is non-nil, and the comment there is explicit - "some backends hang when this stream is opened against them (Consume backend notifications in vMCP and propagate list_changed #5748 R3), so it must stay opt-in" - butserver.gobuilds a sink for every session, so that gate can't be reached from a running deploymentWithBackendInitTimeoutexists but nothing outside tests ever called it, so init was pinned to the 30s default, andoperational.timeoutsonly ever raises it (if requestTimeout > initTimeout)This PR does both halves, because they're the same bug from two ends and they touch the same files:
operational.listChangedwithenabledanddisabledWorkloads, plussession.WithListChangedFilter, a per-workload predicate in the same shape as the resolvers already on the factory. Returning false drops the sink for that backend alone, which is what stops the connector opening the stream. This removes the dependencyoperational.timeouts.backendInit, wired to the existing option, and an explicitly configured value is now authoritative so a longer workload request timeout no longer extends it. This bounds what's leftAn excluded backend loses live propagation and nothing else. Its tools are still aggregated and callable, and they refresh on the next session.
I originally sent these as #6633 and #6635. Folded them together after they conflicted with each other - they share 7 files including the generated CRDs, and whichever landed first would have forced a rebase and full regeneration of the other. #6635 is closed in favour of this.
Type of change
Test plan
task test)task lint-fix)go test ./pkg/vmcp/...and the operator unit tests pass.golangci-lint run ./pkg/vmcp/...is clean at 0 issues. Regenerated withtask operator-generate,task operator-manifestsandtask crdref-gen;task genandtask license-checkleave the tree clean. I also ran the CRD schema check locally with the same SHA-pinnedcrd-schema-checkerand flags CI uses, against the v0.48.0 baseline: exit 0, Compatible.New tests.
TestSessionFactory_ListChangedFilterasserts the connector receives a sink for subscribed backends and nil for excluded ones, since that nil is the whole mechanism, plus a case proving a nil-sink caller is unaffected.TestListChangedFilterandTestBackendInitTimeoutcover the config to behaviour mapping, includingenabled: falsewinning over an exclusion list.TestSessionFactory_ExplicitBackendInitTimeoutIsNotExtendedcovers the new precedence andTestSessionFactory_DefaultBackendInitTimeoutIsExtendedkeeps the old one under test.Manual: hit this on 0.47.1 against Grafana Cloud's hosted MCP, which is stateless and mandates per-user auth. Its own audit log shows the handshake succeeding and then the stream sitting there until the client gives up at exactly 30s:
vMCP's own log stops after
creating session-scoped backendsand never reaches theFailed to initialise backendwarning, because the client has already gone.Worth noting why that backend can't dodge this on its own: it's Modern, so
initOneBackendwould skip the connect entirely, but the revision probe runs unauthenticated from the health monitor, gets a 401, andprobeRevisionquite reasonably leaves a 401 uncached as a transient auth blip. So it never classifies, and because no session ever completes, no authenticated call reachesdispatchto warm the cache either.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.listChangedandtimeouts.backendInitare new optional fields. Nothing is removed or retyped.disabledWorkloadscarrieslistType=setfor the SSA tag the checker requires, matchingMCPGroup.Servers.Does this introduce a user-facing change?
Yes. Two new optional settings under
operational:listChanged, to exclude backends from live list_changed propagation, andtimeouts.backendInit, to cap how long session initialization waits for a single backend. Unset keeps today's behaviour.Special notes for reviewers
Two calls worth your attention.
I made list_changed exclusion config rather than automatic on purpose - I couldn't find a signal that reliably separates "will never service this stream" from "is just slow right now", and guessing wrong silently drops propagation. If you'd rather infer it, or put the switch somewhere other than
operational, happy to move it.The precedence flip is deliberate: an explicit
WithBackendInitTimeoutis now a cap rather than a floor.TestNewSessionFactory_WorkloadTimeoutExtendsBackendInitasserted the old contract, so I split it in two rather than delete it. If you'd rather keep the old precedence and have the config field only bite alongside a loweredtimeouts.default, say so and I'll rework it, though that combination can't express "generous request budget, tight handshake budget" which is the case I need.The deeper fix is presumably for the stream to not gate
initializeat all, but that sits in mcpcompat/go-sdk rather than here, so this is the smallest thing that makes session init independent of it.