Skip to content

Honor configured vMCP backend timeouts - #6411

Open
christensenjairus wants to merge 3 commits into
stacklok:mainfrom
christensenjairus:fix-vmcp-configured-timeouts
Open

Honor configured vMCP backend timeouts#6411
christensenjairus wants to merge 3 commits into
stacklok:mainfrom
christensenjairus:fix-vmcp-configured-timeouts

Conversation

@christensenjairus

@christensenjairus christensenjairus commented Aug 21, 2026

Copy link
Copy Markdown

Summary

vMCP accepts the documented operational.timeouts.default and perWorkload settings, but it did not use them for backend MCP calls. Backend clients kept their 30-second default. A separate 30-second server WriteTimeout could also close a POST before a slow backend returned anything.

This change resolves the configured timeout for each workload and passes it to both client paths, including initialization and SSE operations. It also pauses the server write deadline while the backend call runs, then starts a bounded deadline when a non-streaming response begins.

Fixes #6410

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing

Validation details:

  • Race-enabled tests passed for pkg/vmcp/client, pkg/vmcp/session, pkg/vmcp/session/internal/backend, pkg/vmcp/cli, pkg/vmcp/server, and pkg/transport/middleware.
  • task test ran the full race-enabled package suite. The only failure was pkg/secrets/keyring/TestCompositeProvider_RealProviders; this host has no usable OS keyring provider.
  • task test-e2e ran until its 20-minute suite timeout. It passed 58 specs, including the vMCP CLI feature tests. Six Git skill tests failed because their local Git HTTP server returned 404, which the API reported as 502. The bulk-workload spec was running when the global timeout expired. The harness removed its test workloads afterward.
  • task lint-fix, task lint, go vet ./..., task build, and git diff --check passed.
  • For a manual check, I ran the same slow Elastic query through stock and patched vMCP. Stock vMCP closed the connection after about 31.7 seconds. This build returned HTTP 200 after 75.4 seconds with a 240s per-workload timeout.

API compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

The default behavior does not change when no operational timeout is configured. The variadic duration argument keeps WriteTimeout source-compatible.

Changes

File Change
pkg/vmcp/cli, pkg/vmcp/client Resolve default and per-workload timeouts and configure the core clients
pkg/vmcp/session Apply timeouts to Legacy sessions and initialization
pkg/transport/middleware, pkg/vmcp/server Pause the request deadline during backend work and bound non-streaming response writes
*_test.go Test resolution, clients, connectors, initialization, middleware, and the full handler path

Does this introduce a user-facing change?

Yes. vMCP now honors its documented backend timeout settings. Configurations without timeout settings keep the current defaults.

Special notes for reviewers

An SSE connection can stay open indefinitely, but each MCP operation on that connection uses the configured backend timeout. Session initialization keeps its existing allowance unless the workload timeout is longer.

Apply operational timeout defaults and per-workload overrides to Modern and Legacy backend operations, including SSE operation contexts and session initialization.

Defer the MCP POST write deadline during backend computation, then restore a bounded deadline when a non-streaming response begins.

Signed-off-by: Jairus Christensen <christensenjairus@gmail.com>
@christensenjairus

Copy link
Copy Markdown
Author

Sorry, codex did more than I asked of it 😅. Sorry for requesting each of you directly. I'll humanize this.

@christensenjairus

Copy link
Copy Markdown
Author

From the documentation & from what I can see poking around, it seems this was legitimately missing. The 30s WriteTimeout seems undocumented & unconfigurable.

I caught this as I was using a remote mcp (elastic) that takes roughly ~100s to start writing a response, and nothing in the current implementation can get vmcp to wait that long for the mcp to start writing before it closes the connection.

The fix here works for me & I tried to keep it conservative.

@reyortiz3

Copy link
Copy Markdown
Collaborator

Thanks for tackling this, @christensenjairus — the write-up is thorough and the test coverage across the client, session, and middleware layers is genuinely nice to see, especially the paired timeout/success cases that guard against the old hardcoded 30s accidentally satisfying the new tests. The WriteTimeout middleware rework (suspend-then-rearm) is a clean way to handle this without touching the SSE path.

One correctness issue I'd like resolved before merging:

Per-workload timeout can unintentionally shrink the backend-init allowance, in pkg/vmcp/session/factory.go (initOneBackend) interacting with pkg/vmcp/session/internal/backend/mcp_session.go (NewHTTPConnector's returned closure).

  • factory.go computes initTimeout := max(backendInitTimeout, requestTimeoutResolver(workloadID)), so on its own it can only extend the init deadline — matching the doc comment's promise that "the shorter configured value never reduces an explicit initialization allowance."
  • But NewHTTPConnector's closure re-wraps unconditionally: requestCtx, cancel := context.WithTimeout(ctx /* = bCtx */, requestTimeout), and uses requestCtx for both createMCPClient and initAndQueryCapabilities. context.WithTimeout always takes the earlier deadline, so when a workload's configured timeout is shorter than backendInitTimeout (30s by default), that shorter value silently becomes the effective init deadline — the opposite of the documented guarantee.
  • Since WithBackendInitTimeout is never called from production code (only from tests), this means any perWorkload override shorter than 30s — intended only to bound ordinary tool calls on a fast backend — would also cut short that backend's handshake, risking spurious partial-init failures for backends whose init round trip takes longer than the tightened per-call timeout.
  • This slipped through because TestNewSessionFactory_WorkloadTimeoutExtendsBackendInit (and the pre-existing TestNewSessionFactory_BackendInitTimeout) exercise factory.go's math via a fake connector, bypassing backend.NewHTTPConnector — so the double-timeout interaction has no test covering the "shrink" direction.

Would you mind either having NewHTTPConnector's closure only tighten the deadline when requestTimeout is longer than what's left on the incoming context, or having init rely on the already-widened ctx as-is and reserve requestTimeout for the per-call operations (as already done in mcpSession.CallTool/ReadResource/GetPrompt)? A test with a real connector and a workload timeout shorter than backendInitTimeout would pin down the desired behavior too.

Everything else looks solid — happy to take another pass once this is addressed.

Use the factory's widened initialization context when connecting persistent backends, while retaining the configured timeout for ordinary session operations.

Cover the shorter request-timeout case with the real HTTP connector.

Signed-off-by: Jairus Christensen <christensenjairus@gmail.com>
Signed-off-by: Jairus Christensen <christensenjairus@gmail.com>
@christensenjairus

Copy link
Copy Markdown
Author

@reyortiz3 Thanks for the review! I believe I've addressed your concerns.

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 ignores configured backend timeouts

2 participants