Skip to content

test(vmcp): pin that the health path never opens a standalone GET stream - #6520

Open
jstar0 wants to merge 1 commit into
stacklok:mainfrom
jstar0:fix/6497-health-probe-no-standalone-get
Open

test(vmcp): pin that the health path never opens a standalone GET stream#6520
jstar0 wants to merge 1 commit into
stacklok:mainfrom
jstar0:fix/6497-health-probe-no-standalone-get

Conversation

@jstar0

@jstar0 jstar0 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Relates to #6497.

@blkt — you were right that the reported behaviour no longer applies to main, and I verified it before writing anything:

  • health.NewHealthChecker uses BackendClient.ListCapabilities, not a bare GET (pkg/vmcp/health/checker.go).
  • The only GET vMCP makes to a backend is the standalone server→client SSE stream, and newStreamableHTTPClient appends transport.WithContinuousListening() only when fwd != nil && forwarding — the tools/call path. Aggregation, list, read, prompt, complete and the health check all get the plain client.

So there is no bug left to fix here, and I'm not proposing a behaviour change. What is missing is a pin. The property that closes #6497a backend that rejects GET is still healthy — is currently implied by a condition in newStreamableHTTPClient and by comments; nothing fails if that condition is loosened. revision_realbackend_test.go pins the neighbouring property (the version gate suppressing the stream on the forwarding path), which is the opposite arm.

This PR adds the regression the issue describes, in the style of that file: a real go-sdk stateful (Legacy) streamable-HTTP backend behind a handler that answers every GET with Tableau's exact 400 Invalid or missing session ID and records the method of every request.

The assertion is deliberately two-sided:

require.NoError(t, err, "…the health check speaks MCP over POST")
assert.NotContains(t, methods, http.MethodGet, "the health path must not open a standalone SSE GET stream…")

The NotContains half is the one with teeth. Appending WithContinuousListening() unconditionally in newStreamableHTTPClient does not make ListCapabilities return an error against this backend — the call still round-trips — it only starts a GET that the backend rejects:

[]string{"POST", "POST", "POST", "GET", "POST", "POST", "DELETE"} should not contain "GET"

That is what a regression here would look like in production too: not a loud failure, but a stream opened against every session-requiring backend, which is what put Tableau into unavailable and dropped its tools from tools/list in the first place.

I deliberately did not touch ping. Your correction stands — it is neither universally sessionless nor unauthenticated, and 2026-07-28 removes it — so it is the wrong health primitive, and ListCapabilities is already the protocol-version-aware one.

Verification: go test ./pkg/vmcp/client/ ./pkg/vmcp/health/ passes; golangci-lint run ./pkg/vmcp/client/... reports 0 issues.

If you'd rather close #6497 as already-fixed without carrying a test for it, say the word and I'll close this.

Issue stacklok#6497 reported a healthy Tableau MCP backend being marked unavailable
because the vMCP health probe used a bare HTTP GET, which that backend
rejects with 400 for lack of a session id. Current main no longer probes
with GET: the health check is BackendClient.ListCapabilities, and
newStreamableHTTPClient enables transport.WithContinuousListening — the
standalone server->client SSE stream, the only GET vMCP makes — solely on
the forwarding tools/call path.

Nothing pinned that, so add the regression the issue describes: a real
go-sdk stateful streamable-HTTP backend behind a handler that answers every
GET with Tableau's 400, asserting that ListCapabilities both succeeds and
issues no GET at all.

Verified to have teeth: appending WithContinuousListening unconditionally in
newStreamableHTTPClient makes the test fail on the GET assertion.
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.38%. Comparing base (6b40bf3) to head (24049c7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6520      +/-   ##
==========================================
- Coverage   78.43%   78.38%   -0.06%     
==========================================
  Files         776      776              
  Lines       76094    76094              
==========================================
- Hits        59685    59643      -42     
- Misses      16404    16446      +42     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jstar0

jstar0 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

CI note so nobody has to re-derive this: the one red check, E2E Test Lifecycle (kindest/node:v1.33.7), is unrelated to this PR.

The single real failure is

[FAIL] VirtualMCPServer Health-Driven tools/list_changed
       [It] notifies a connected session when a backend recovers, without reconnect
       test/e2e/thv-operator/virtualmcp/virtualmcp_health_list_changed_test.go:275

and the other five entries in the summary are [INTERRUPTED], i.e. fail-fast tearing down the remaining specs.

Two things say this isn't mine:

  • The same commit passes the same suite on the other two node images — kindest/node:v1.34.3 and kindest/node:v1.35.1 are both green. 42 of 43 checks pass.
  • This PR adds one file, pkg/vmcp/client/getreject_realbackend_test.go, and nothing else. It is a _test.go file, so it is not compiled into the operator or the vMCP binary the E2E suite runs at all.

Happy to rebase or push an empty commit if you'd like the job re-run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vMCP health probe uses HTTP GET, breaking backends that only accept POST (e.g. Tableau MCP)

1 participant