From 6e5a3ff23959c3fbfc9cf6bd8a7024c8b1d57f4a Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 4 Aug 2026 12:29:25 +0300 Subject: [PATCH 1/2] Pin elicitation fail-fast without a standalone stream A downstream client that advertised the elicitation capability but holds no open standalone SSE stream passes go-sdk's capability gate, yet the elicitation cannot be delivered: under the shim's JSONResponse transport the go-sdk routes server->client requests to the standalone stream, and a missing stream rejects the write ("rejected by transport: stream not connected or already closed"). The mid-call tools/call fails fast with a tool error rather than hanging to the deadline. TestForwarding_Elicitation_AdvertisedButNoStream_FastFails pins that behavior end to end through vMCP (the timing assertion is on the call itself, which returns in single-digit ms; the suite's 30s teardown cost is a pre-existing idle-connection artifact of the forwarding fixtures, not the path under test). The no-listen downstream-client variant force-closes idle connections at teardown so its own cleanup does not add to that cost. Also documents the two delivery constraints in the vMCP architecture doc: the advertised-but-no-stream fail-fast, and cross-pod origination requiring session affinity (a server->client request can only be delivered by the replica holding the client's standalone stream; the durable fix is the 2026-07-28 revision's MRTR, which has no stream-locality requirement). Part of #5975 --- docs/arch/10-virtual-mcp-architecture.md | 26 +++++++ ...forwarding_realbackend_integration_test.go | 68 +++++++++++++++++-- 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 70525f3c4d..766d138444 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -806,6 +806,32 @@ for what a Modern caller gets instead. **Known limitation (logging level)**: forwarded backend logging is not yet filtered to the downstream client's requested level. On Legacy, vMCP requests debug-level logging from the backend (`logging/setLevel`) so it emits `notifications/message`, and every such notification is forwarded — the downstream client's own `logging/setLevel` preference is not applied to the relayed stream. The same is true on Modern (2026-07-28), where the RPC is removed and the level rides per-request in `_meta["io.modelcontextprotocol/logLevel"]`: vMCP strips that reserved per-hop key from the downstream request and overlays its own (`debug`, when forwarding is bound) on the backend hop, so a Modern client's per-request level preference is likewise not honored — the relay runs at debug either way. +**Known limitation (advertised-but-no-stream elicitation fails fast)**: a client +that advertised the `elicitation` capability but holds **no open standalone SSE +stream** passes go-sdk's capability gate, yet the elicitation cannot be +delivered — under the shim's `JSONResponse` transport the go-sdk routes +server→client requests to the standalone stream, and a missing stream rejects +the write ("rejected by transport: stream not connected or already closed"). +The mid-call `tools/call` therefore fails fast with a tool error instead of +hanging to the deadline (pinned by +`TestForwarding_Elicitation_AdvertisedButNoStream_FastFails` in +`pkg/vmcp/server`). A cleaner pre-dispatch refusal awaits an upstream mcpcompat +accessor for stream presence (#5975). + +**Known limitation (cross-pod origination needs session affinity)**: a +server→client request can only be delivered by the replica currently holding +the client's standalone SSE stream. If the `tools/call` executes on replica A +but the client's GET stream is pinned to replica B, an elicitation or sampling +request originated from A cannot reach the client — the shim loads the go-sdk +session bound to *its* pod and has no cross-replica delivery channel for +request/response (only notifications rehydrate cross-replica). Multi-replica +deployments that rely on mid-call elicitation/sampling therefore need **session +affinity** at the load balancer pinning the standalone stream and the tool +calls to the same replica. The durable fix is the 2026-07-28 revision itself: +it replaces server-initiated requests with client-polled MRTR, which has no +stream-locality requirement — so this constraint is documented rather than +engineered around (#5975, #5743). + **Known limitation (resource-template authorization)**: a resource template is advertised on the template-string entity (e.g. `file:///logs/{date}.txt`), but a concrete read is admission-checked on the **expanded** URI (e.g. `file:///logs/2025-01-01.txt`). Operators should therefore author resource authorization policies against concrete URI patterns, not the template string. ## Two-Boundary Authentication diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index 3d7b2ec435..741049dbef 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -286,6 +286,16 @@ type downstreamClient struct { // registers an OnNotification collector. func newDownstreamClient(ctx context.Context, t *testing.T, vmcpURL string, withHandlers bool) *downstreamClient { t.Helper() + return newDownstreamClientOpts(ctx, t, vmcpURL, withHandlers, true) +} + +// newDownstreamClientOpts is newDownstreamClient with explicit control over +// the standalone SSE stream (listen): pass listen=false to model a client that +// advertises elicitation/sampling but never opens the standalone stream. +func newDownstreamClientOpts( + ctx context.Context, t *testing.T, vmcpURL string, withHandlers, listen bool, +) *downstreamClient { + t.Helper() dc := &downstreamClient{ notifCh: make(chan mcpmcp.JSONRPCNotification, 8), @@ -325,12 +335,15 @@ func newDownstreamClient(ctx context.Context, t *testing.T, vmcpURL string, with } hc, pinRT := newLegacyPinnedHTTPClient() + transportOpts := []transport.StreamableHTTPCOption{ + transport.WithHTTPBasicClient(hc), + } + if listen { + transportOpts = append(transportOpts, transport.WithContinuousListening()) + } c, err := client.NewStreamableHttpClientWithOpts( vmcpURL, - []transport.StreamableHTTPCOption{ - transport.WithContinuousListening(), - transport.WithHTTPBasicClient(hc), - }, + transportOpts, clientOpts, ) require.NoError(t, err) @@ -345,6 +358,13 @@ func newDownstreamClient(ctx context.Context, t *testing.T, vmcpURL string, with require.NoError(t, c.Start(ctx)) t.Cleanup(func() { _ = c.Close() }) + if !listen { + // Without a standalone stream, client Close leaves the HTTP client's + // idle keep-alive connection to the test server open, and + // httptest.Server.Close then blocks ~30s waiting for it. Force-close + // idle connections at teardown instead. + t.Cleanup(hc.CloseIdleConnections) + } _, err = c.Initialize(ctx, mcpmcp.InitializeRequest{ Params: mcpmcp.InitializeParams{ @@ -572,6 +592,46 @@ func TestForwarding_Elicitation_NoDownstreamCapability(t *testing.T) { assert.True(t, res.IsError, "backend elicitation must fail when downstream lacks the capability") } +// TestForwarding_Elicitation_AdvertisedButNoStream_FastFails pins the runtime +// twin of TestForwarding_Elicitation_NoDownstreamCapability (#5975): a client +// that ADVERTISED the elicitation capability but holds NO open standalone SSE +// stream passes go-sdk's capability gate, yet the elicitation cannot be +// delivered — under JSONResponse the go-sdk routes server->client requests to +// the standalone stream, and a missing stream rejects the write +// ("rejected by transport: stream not connected or already closed"). +// +// The assertion is timing-structural, not string-matching: with a generous +// outer deadline, the call must fail as a tool error FAR below it (a hang-to- +// timeout regression blows the full deadline instead). This documents and pins +// the fail-fast until an upstream mcpcompat stream-presence accessor lets vMCP +// fail before dispatch with a cleaner error. +func TestForwarding_Elicitation_AdvertisedButNoStream_FastFails(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) + defer cancel() + + backendURL := startForwardingBackend(t) + vmcpTS := newRealTestServer(t, backendURL) + // withHandlers=true (capability advertised) but listen=false (no stream). + dc := newDownstreamClientOpts(ctx, t, vmcpTS.URL+"/mcp", true, false) + + start := time.Now() + res, err := dc.c.CallTool(ctx, mcpmcp.CallToolRequest{ + Params: mcpmcp.CallToolParams{Name: fwdElicitTool}, + }) + elapsed := time.Since(start) + + // Same structural shape as the capability-gate twin — see the rationale + // there — plus the timing assertion that distinguishes fail-fast from hang. + require.NoError(t, err, + "the call must round-trip on the live session, not die at transport level") + require.NotNil(t, res) + assert.True(t, res.IsError, + "backend elicitation must fail when the downstream holds no standalone stream") + assert.Less(t, elapsed, 15*time.Second, + "elicitation without a standalone stream must fail fast, not hang to the deadline") +} + // samplingClient is a downstream client whose sampling handler returns a // DISTINGUISHABLE summary+model and counts its own invocations, used to prove // per-session isolation of forwarded server->client sampling. From 3b49b8957e92aa6cc2971028ab25036081062c2f Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 4 Aug 2026 15:09:47 +0300 Subject: [PATCH 2/2] Address amirejaz's review on cleanup ordering and test conventions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cleanup ordering: register CloseIdleConnections BEFORE c.Close() so LIFO teardown runs it after the session-terminating DELETE (which opens a fresh connection over hc that would otherwise linger). Comment corrected to note the suite's ~30s httptest.Server.Close stall is the vMCP-side backend-client connection, which this helper does not own — verified by the identical stall on the pre-existing RealBackend tests. - Derive the fail-fast bound from forwardingRealBackendTimeout/4 instead of a magic 15s, per the file's no-independent-timers convention. - Add the Legacy-pinning / vacuous-pass rationale note to the new test, matching its siblings. --- ...forwarding_realbackend_integration_test.go | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index 741049dbef..000caea625 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -357,14 +357,18 @@ func newDownstreamClientOpts( }) require.NoError(t, c.Start(ctx)) - t.Cleanup(func() { _ = c.Close() }) if !listen { - // Without a standalone stream, client Close leaves the HTTP client's - // idle keep-alive connection to the test server open, and - // httptest.Server.Close then blocks ~30s waiting for it. Force-close - // idle connections at teardown instead. + // Force-close this downstream client's idle keep-alive connections at + // teardown. Registration order matters: t.Cleanup runs + // last-added-first, and c.Close() sends the session-terminating DELETE + // over hc, opening a fresh connection that goes idle — so + // CloseIdleConnections must be registered FIRST to run AFTER c.Close(). + // Note the vMCP server holds its own backend-client connection, so the + // suite's ~30s httptest.Server.Close stall persists regardless; this + // only covers the connection this helper owns. t.Cleanup(hc.CloseIdleConnections) } + t.Cleanup(func() { _ = c.Close() }) _, err = c.Initialize(ctx, mcpmcp.InitializeRequest{ Params: mcpmcp.InitializeParams{ @@ -600,6 +604,11 @@ func TestForwarding_Elicitation_NoDownstreamCapability(t *testing.T) { // the standalone stream, and a missing stream rejects the write // ("rejected by transport: stream not connected or already closed"). // +// Legacy-pinned like its siblings, for the same vacuous-pass reason: on Modern +// the call fails with the sessionless error regardless of stream state, so a +// Modern run would satisfy the assertions without exercising the delivery +// path this test exists for. +// // The assertion is timing-structural, not string-matching: with a generous // outer deadline, the call must fail as a tool error FAR below it (a hang-to- // timeout regression blows the full deadline instead). This documents and pins @@ -628,7 +637,7 @@ func TestForwarding_Elicitation_AdvertisedButNoStream_FastFails(t *testing.T) { require.NotNil(t, res) assert.True(t, res.IsError, "backend elicitation must fail when the downstream holds no standalone stream") - assert.Less(t, elapsed, 15*time.Second, + assert.Less(t, elapsed, forwardingRealBackendTimeout/4, "elicitation without a standalone stream must fail fast, not hang to the deadline") }