Add Redis-backed mixed-era vMCP e2e coverage - #6056
Merged
Merged
Conversation
jhrozek
requested review from
ChrisJBurns,
JAORMX,
blkt,
jerm-dro,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
July 27, 2026 20:00
JAORMX
approved these changes
Jul 27, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6056 +/- ##
==========================================
- Coverage 72.30% 72.27% -0.04%
==========================================
Files 722 724 +2
Lines 75138 75330 +192
==========================================
+ Hits 54327 54443 +116
- Misses 16934 17009 +75
- Partials 3877 3878 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
reyortiz3
approved these changes
Jul 27, 2026
11 tasks
PR #5981 removed two specs from the single-server dual-era k8s test because they cannot work on one MCPServer: a single go-sdk backend cannot be both stateless (for Modern per-request clients) and session-issuing (for Legacy clients) at once. PR #6006 then landed the vMCP cross-generation bridge, but neither of its tiers configures Redis, so the session-store dimension of the bridge stayed unproven: nothing exercised a Legacy client's session metadata living in Redis while a Modern backend shared the group, and nothing exercised what a Redis outage does to a mixed-era deployment. Add an operator-tier spec over a mixed Legacy+Modern yardstick backend set behind one vMCP with Redis session storage. It asserts the asymmetry a single-server test could not express: a Redis outage 503s the Legacy session path while the Modern stateless path keeps serving, because vMCP classifies client era in middleware ahead of the SDK session layer and dispatchModern never reads the session store. Coverage: concurrent Legacy-session and Modern-stateless traffic with no cross-delivery; Redis holding a backend-session key for the Legacy backend only; and outage to 503 to recovery, with the wiped pre-outage session correctly reported terminated. Two constraints are documented in the file because both cost a debug cycle. yardstick's echo tool schema-validates input against ^[a-zA-Z0-9]+$ and reports a violation as a successful result with isError set, so resp.Error being nil is not proof a call worked. And a go-sdk/mcpcompat client cannot open a Legacy session against a Modern-dispatch-enabled vMCP at all: Connect is Modern-first, so server/discover routes to dispatchModern, the client negotiates Modern, gets no session, and then 404s on subscriptions/listen. This spec therefore drives all traffic through the raw client, which pins the era per request. Runs at one replica rather than the multiple replicas the issue suggests. WithSessionIdManager is wired unconditionally, so one replica exercises the Redis store just as fully; two would add only the rehydration path, whose transient-error branch returns 404 instead of 503 unless the pod holds a cached rehydration, making the outage assertion nondeterministic. Cross-pod Redis session sharing is already covered by virtualmcp_redis_session_test.go. Closes #6008 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
force-pushed
the
redis-vmcp-dual-era-e2e
branch
from
July 28, 2026 06:38
acff2cd to
018da30
Compare
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MCPServer: a single go-sdk backend cannot be both stateless (serving Modern per-request clients) and session-issuing (serving Legacy clients) at the same time. The commit asked whoever landed the vMCP cross-generation bridge to re-add them as vMCP coverage.sessionStorage: redis. The property it pins is the asymmetry a single-server test could not express: a Redis outage 503s the Legacy session path while the Modern stateless path keeps serving, because vMCP classifies client era in middleware ahead of the SDK session layer anddispatchModernnever reads the session store.scaleRedishelper deleted in5fa67123b, name-parameterized this time so scaling one spec's Redis cannot disturb another's under--procs=8(the original hardcoded a sharedredisDeployment, which is why it needed its own namespace).Closes #6008
Type of change
Test plan
task test-e2e)task lint-fix)Run against a Kind cluster created with
kind-setup-e2e(the NodePort mappings are required):3 Passed | 0 Failed, including after rebasing onto currentmainand rebuilding/redeploying all three images.--procs=8suite run, so they do not collide with the other 146 specs.task lint-fix: 0 issues repo-wide.Negative checks — the part that matters. Both were run to confirm the specs are not passing vacuously:
STATELESS=falseon the Modern backendmodern backend …: want revision "2026-07-28", got "2025-11-25", both backends LegacyTOOLHIVE_VMCP_MODERN_STATELESS=falsetools/callleg failsWithout those two gates a forgotten env var would silently collapse the "mixed" set and every spec would still pass while testing one era twice — a failure mode that has already happened once in this area.
API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.gotest/e2e/thv-operator/virtualmcp/mcpserver_scaling_test.goscaleRedis(name, replicas)next to the other Redis helpers.Does this introduce a user-facing change?
No.
Special notes for reviewers
Deviation from the issue text: this runs at one replica, not multiple.
WithSessionIdManageris wired unconditionally (pkg/vmcp/server/server.go:570), so one replica exercises the Redis store just as fully. Two replicas would add only the rehydration path, and that path is what makes the 503 nondeterministic: on a non-owning podserveRehydratedreturns 503 only if that pod happens to hold a cached rehydration, otherwise 404Invalid session ID. Cross-pod Redis session sharing is already covered byvirtualmcp_redis_session_test.goContext 2, soreplicas=2here would buy coverage that exists elsewhere and pay for it with port-forward pinning to work around a coin flip we'd have introduced ourselves. Happy to switch if reviewers disagree.Two constraints cost a debug cycle each and are documented in the file — worth knowing before editing it:
echotool schema-validatesinputagainst^[a-zA-Z0-9]+$, and reports a violation as a successful JSON-RPC result withisError: true. Soresp.Error == nilis not proof a call worked, and any nonce must be run-together alphanumeric (round0req3, neverround-0-req-3).Connectis Modern-first, soserver/discoverroutes todispatchModern(pkg/vmcp/server/classification.go:97-110), the client negotiates Modern, gets no session, and then 404s on thesubscriptions/listenstream mcpcompat'sInitializeunconditionally opens — surfaced asErrSessionMissingafterInitializeapparently succeeded. That rules outCreateInitializedMCPClient/WaitForExpectedTools/ToolsContainAllhere, so all traffic goes throughe2e.RawMCPClient, which pins the era per request. Shared helpers were left untouched; they remain correct for a dispatcher-off vMCP.The Redis negative assertion (
Not(HaveKey("vmcp.backend.session." + modernBackend))) is deterministic in both revision-cache states, and the comment explains why: the warm path skips the connect viaisKnownModern(pkg/vmcp/session/factory.go:281-287), and even off that pathpopulateBackendMetadata(factory.go:414-425) only writes the key when the backend reports a non-empty session ID, which a stateless backend never does.Unrelated observation while verifying: the operator E2E suite is flaky under
--procs=8on a single-node Kind cluster independent of this change — a baseline run with this file removed failed more specs (8 vs 6) and on a different root spec. Not addressed here.Generated with Claude Code