fix(vmcp): exempt SSE connections from WriteTimeout to prevent drops#4167
fix(vmcp): exempt SSE connections from WriteTimeout to prevent drops#4167
Conversation
yrobla
requested review from
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
github-actions
bot
added
size/S
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04169cdc72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR fixes long-lived SSE (Streamable HTTP) connections in the vMCP server being dropped after ~30s by removing the global http.Server.WriteTimeout and replacing it with a per-request write deadline applied only to non-SSE requests.
Changes:
- Removed
http.Server.WriteTimeoutfrom the vMCP server to avoid killing long-lived SSE (GET) streams. - Added
writeTimeoutMiddlewareusinghttp.ResponseController.SetWriteDeadlineto enforce per-request write deadlines on non-GET requests. - Added unit + integration tests to validate deadline-setting behavior and confirm SSE streams survive past the timeout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/vmcp/server/server.go | Removes global WriteTimeout; adds per-request write-deadline middleware and wires it into the MCP handler chain. |
| pkg/vmcp/server/write_timeout_middleware_test.go | Adds tests verifying deadline behavior by method and an end-to-end SSE survivability test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4167 +/- ##
========================================
Coverage 69.13% 69.13%
========================================
Files 462 467 +5
Lines 46554 46808 +254
========================================
+ Hits 32185 32362 +177
+ Misses 11897 11894 -3
- Partials 2472 2552 +80 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/S
There was a problem hiding this comment.
Pull request overview
This PR addresses long-lived SSE connections being dropped due to Go’s http.Server.WriteTimeout applying an absolute response deadline, impacting the vMCP Streamable HTTP transport.
Changes:
- Adds a
writeTimeoutMiddlewareintended to exempt GET (SSE) requests from write timeouts by clearing the write deadline viahttp.ResponseController.SetWriteDeadline(time.Time{}). - Wires the middleware into the MCP handler chain.
- Adds unit + TCP-level integration tests validating SSE streams survive past a configured
WriteTimeout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/vmcp/server/server.go | Introduces and wires writeTimeoutMiddleware to clear write deadlines for SSE GET requests. |
| pkg/vmcp/server/write_timeout_middleware_test.go | Adds unit tests for deadline behavior by method and an integration-style test ensuring SSE survives beyond WriteTimeout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yrobla
changed the title
github-actions
bot
added
size/S
github-actions
bot
added
size/M
There was a problem hiding this comment.
Pull request overview
This PR addresses long-lived MCP Streamable HTTP SSE connections being dropped due to Go’s http.Server.WriteTimeout, by introducing a per-request write-deadline middleware that clears the deadline for qualifying SSE GET requests while keeping server-level timeouts enabled for other traffic.
Changes:
- Add a transport-level
WriteTimeoutmiddleware that clears the write deadline for qualifying SSE requests viahttp.ResponseController.SetWriteDeadline(time.Time{}). - Wire the middleware into the vMCP server handler chain for MCP routes while retaining
http.Server.WriteTimeout. - Add unit tests plus a TCP-level integration test to validate SSE streams aren’t terminated by server
WriteTimeout.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/vmcp/server/server.go |
Applies the new write-deadline middleware to the MCP handler chain and updates timeout-related comments. |
pkg/transport/middleware/write_timeout.go |
Introduces the per-request write-deadline management middleware and SSE request detection. |
pkg/transport/middleware/write_timeout_test.go |
Adds unit and integration tests covering deadline setting/clearing behavior and SSE stream survivability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
size/M
There was a problem hiding this comment.
Pull request overview
Fixes vMCP Streamable HTTP SSE connections being dropped by Go’s http.Server.WriteTimeout by clearing the per-connection write deadline for qualifying SSE GET requests, while keeping the server-level timeout protection for non-SSE traffic.
Changes:
- Add
transportmiddleware.WriteTimeoutto clear the write deadline for SSE GET requests to the MCP endpoint path. - Wire the middleware into the vMCP handler chain while retaining
http.Server.WriteTimeout. - Add unit + integration coverage for the middleware and refactor real-backend test helpers to allow configuring
httptest.Server.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/server/server.go | Wires write-timeout middleware into MCP handler chain and updates timeout documentation. |
| pkg/transport/middleware/write_timeout.go | Introduces middleware that clears write deadlines for qualifying SSE requests. |
| pkg/transport/middleware/write_timeout_test.go | Adds unit tests (deadline clearing) and an end-to-end TCP test for SSE survivability past WriteTimeout. |
| pkg/vmcp/server/write_timeout_integration_test.go | Adds integration tests against the full vMCP handler/server configuration. |
| pkg/vmcp/server/session_management_realbackend_integration_test.go | Refactors helper to return a handler so tests can control httptest.Server config (e.g., WriteTimeout). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
size/M
github-actions
bot
added
size/M
There was a problem hiding this comment.
Pull request overview
This PR fixes vMCP Streamable HTTP SSE connections being dropped by Go’s http.Server.WriteTimeout by introducing a targeted middleware that clears the write deadline only for qualifying SSE GET requests to the MCP endpoint, while keeping WriteTimeout protections for all other routes.
Changes:
- Added
transportmiddleware.WriteTimeoutto clear per-request write deadlines for qualifying SSE connections. - Wired the new middleware into the vMCP server’s MCP handler chain while retaining
http.Server.WriteTimeoutglobally. - Added unit and integration tests to verify deadline clearing and end-to-end SSE survivability past server write timeouts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/transport/middleware/write_timeout.go | New middleware that clears write deadlines for qualifying SSE GET requests to the MCP endpoint. |
| pkg/transport/middleware/write_timeout_test.go | Unit tests for qualification logic + TCP-level integration test verifying SSE survives past server WriteTimeout. |
| pkg/vmcp/server/server.go | Applies the new middleware to the MCP handler chain; documents rationale for keeping server-level WriteTimeout. |
| pkg/vmcp/server/write_timeout_integration_test.go | New vMCP integration tests covering SSE survival past WriteTimeout and non-SSE GET rejection behavior. |
| pkg/vmcp/server/session_management_realbackend_integration_test.go | Refactors test setup to expose a reusable handler builder for configurable httptest.Server settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Go's http.Server.WriteTimeout sets an absolute deadline on the entire response duration. For long-lived SSE connections used by the MCP Streamable HTTP transport, this caused streams to be killed after 30s regardless of write activity (see golang/go#16100). Remove WriteTimeout from the http.Server config and replace it with a writeTimeoutMiddleware that uses http.ResponseController.SetWriteDeadline to apply a per-request 30s write deadline only to non-GET (non-SSE) requests. GET requests remain exempt so SSE streams can stay open indefinitely. Tests added: - Unit tests verifying SetWriteDeadline is called/not called by method - Integration test over a real TCP connection confirming an SSE stream survives past the timeout without being cut Fixes #3691
github-actions
bot
added
size/M
| // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package server_test |
There was a problem hiding this comment.
Suggestion: delete this test. It is redundant with the new write_timeout_test.go.
Summary
Go's
http.Server.WriteTimeoutsets an absolute deadline on the entire response duration. For long-lived SSE connections used by the MCP Streamable HTTP transport, this caused streams to be killed after 30s regardless of write activity (see golang/go#16100).Approach: Keep
http.Server.WriteTimeout(it protects all routes from slow/hung non-SSE clients), and addtransportmiddleware.WriteTimeoutinpkg/transport/middlewarethat useshttp.ResponseController.SetWriteDeadline(time.Time{})to clear the write deadline for qualifying SSE connections only. A connection qualifies when all three hold: method is GET,Acceptheader containstext/event-stream, and path matches the MCP endpoint exactly. Non-qualifying requests are left completely untouched —http.Server.WriteTimeoutremains in effect for them.Tests added:
SetWriteDeadlineis called (qualifying SSE) / not called (POST, DELETE, GET without SSE headers, GET on wrong path)WriteTimeout(asserts both elapsed time ≥ stream duration and event count > what's possible before timeout fires)Fixes #3691
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
pkg/transport/middleware/write_timeout.goWriteTimeoutmiddleware — clears write deadline for qualifying SSE connections onlypkg/transport/middleware/write_timeout_test.gopkg/vmcp/server/server.gotransportmiddleware.WriteTimeoutto MCP handler; keephttp.Server.WriteTimeoutfor all other routespkg/vmcp/server/write_timeout_integration_test.gopkg/vmcp/server/session_management_realbackend_integration_test.gonewRealTestHandlerhelper to allowhttptest.Serverconfiguration in testsDoes this introduce a user-facing change?
No — this is an internal fix. SSE streams that were silently dropped after 30s will now remain open.
Special notes for reviewers
The middleware only touches qualifying SSE requests (GET +
Accept: text/event-stream+ matching endpoint path). Non-qualifying requests are intentionally left untouched — resetting the deadline totime.Now().Add(defaultTimeout)would extend it (sincetime.Now()is later than connection-accept time), weakening the protection for health, metrics, and JSON-RPC POST routes. IfSetWriteDeadlinefails on a qualifying SSE connection (e.g., a wrappingResponseWriterthat doesn't implement the interface), aWarnlog is emitted so the gap is visible in observability tooling.