fix(vmcp): bound initialize to healthCheckTimeout - #6380
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Automated fix: rebased onto main (20 commits). Remaining golangci findings are in operator files this PR does not touch (gci/staticcheck SA1019). |
72f5609 to
880e09d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6380 +/- ##
==========================================
- Coverage 78.31% 78.30% -0.02%
==========================================
Files 770 770
Lines 75456 75479 +23
==========================================
+ Hits 59095 59104 +9
- Misses 16356 16370 +14
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e14a014 to
cdf9562
Compare
cdf9562 to
6f61794
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent vMCP client-facing initialize from hanging past typical gateway/client timeouts while /health and the VirtualMCPServer Ready signal remain green, by introducing an overall session initialization budget tied to healthCheckTimeout and documenting the distinction between liveness, readiness, and session init.
Changes:
- Add an overall
sessionInitTimeoutbudget to the vMCP session factory to boundMakeSessioninitialization time (best-effort: slow backends are skipped). - Wire
failureHandling.healthCheckTimeoutinto the session factory as the initialize budget (default remains 10s). - Document Health vs Ready vs
initialize, and propagate updated field descriptions into generated/operator docs and CRDs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/factory.go | Introduces default session init budget + option, applies timeout context, and logs when budget expires. |
| pkg/vmcp/session/default_session_test.go | Adds unit tests for the new session init timeout option and bounding behavior. |
| pkg/vmcp/server/server.go | Clarifies /health as liveness-only and distinguishes it from readiness/session init. |
| pkg/vmcp/config/config.go | Documents that healthCheckTimeout also bounds client session initialize across backends. |
| pkg/vmcp/cli/serve.go | Plumbs failureHandling.healthCheckTimeout into WithSessionInitTimeout. |
| docs/operator/virtualmcpserver-api.md | Adds operator-facing documentation on Health vs Ready vs initialize semantics. |
| docs/operator/crd-api.md | Updates CRD API docs to reflect the expanded meaning of healthCheckTimeout. |
| deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml | Updates chart CRD template descriptions for healthCheckTimeout. |
| deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml | Updates packaged CRD descriptions for healthCheckTimeout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A new session waited on every backend's 30s init timeout, so initialize could hang past gateway limits while Ready and /health stayed OK. Cap MakeSession to healthCheckTimeout (default 10s) and document the three signals. Fixes stacklok#6345
Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Queued backend init must not block on the semaphore after the session budget expires. Warn only on deadline exceeded; log caller cancel at debug. Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
191387a to
02a515e
Compare
Cross-links
Summary
VirtualMCPServer Ready and
GET /healthcould stay OK while a new client'sinitializehung past typical gateway timeouts (12s) with a 0-byte response.Those signals are different operations:
/healthis liveness, Ready is thelast
ListCapabilitiesprobe, and initialize waits on backend handshakes(per-backend 30s, concurrency 10). This does not flip Ready/
/healthoninitialize failure (that would flap). It bounds the handshake.
MakeSessionbudget defaults to 10s (same as CRDhealthCheckTimeout)WithSessionInitTimeoutfromfailureHandling.healthCheckTimeoutwhen set
Fixes #6345
Type of change
Test plan
go test -ldflags=-extldflags=-Wl,-w ./pkg/vmcp/session/ -run 'TestWithSessionInitTimeout|TestNewSessionFactory_SessionInitTimeout|TestNewSessionFactory_BackendInitTimeout|TestNewSessionFactory_AllBackendsFail'healthCheckTimeout;/healthremains liveness-onlyAPI Compatibility
This does not change the CRD schema.
healthCheckTimeoutalready exists; itnow also bounds session initialize.
Does this introduce a user-facing change?
Yes. New vMCP sessions finish initialize within
healthCheckTimeout(default 10s). Slow backends are omitted from that session instead of
blocking the client.
/healthis documented as liveness only.Special notes for reviewers
Direction from #6345: Ready is ListCapabilities, initialize is a different
handshake. Bounding + documenting is the agreed path; a separate SessionReady
signal is left for a follow-up if you want it.