From 00d758288173ba73e6c117516c4d3828769667ba Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 16 Aug 2026 07:59:18 +0300 Subject: [PATCH] mcphttp: test Unwrap for what it actually carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment claimed Unwrap was what kept streaming alive, and the test named after that claim passed with the method deleted. Both were wrong, and wrong for the same reason: cacheWriter has its own Flush, and http.NewResponseController prefers a method on the writer it is handed over one reached by unwrapping, so the flush path never unwraps at all. What Unwrap does carry is everything else the controller offers — the deadlines and Hijack. A long-lived MCP stream is the response that wants its write deadline pushed out. The new test measures that against a real server, because a recorder supports no deadlines either way and would have been vacuous a second time. Found by an independent reviewer that mutated the method away instead of reading the comment. --- mcphttp/cache.go | 20 +++++++++++------- mcphttp/cache_test.go | 47 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/mcphttp/cache.go b/mcphttp/cache.go index 8d38dc43d1c323602becfc77297aaabc1a3dc10e..b233d2178200a25148c3eb6a9e1b1cc232a9b2d7 100644 --- a/mcphttp/cache.go +++ b/mcphttp/cache.go @@ -83,11 +83,17 @@ func (w *cacheWriter) commit() { w.Header().Set("Vary", vary) } -// Unwrap is load-bearing, not boilerplate: it is what keeps the streamable -// transport working through this wrapper. http.NewResponseController follows -// Unwrap to reach the real writer's Flush, and an SSE stream that could not be -// flushed would be a response no client sees until the handler returns — which, -// for a stream, is a response nobody sees at all. Deleting this method breaks -// streaming while leaving every status code and header test green, so -// TestUnwrapReachesTheUnderlyingFlusher exists to fail instead. +// Unwrap is what http.NewResponseController follows to reach the real writer. +// +// It is not what keeps flushing working — this type has its own Flush, and the +// controller prefers a method on the writer it is handed over one reached by +// unwrapping, so the flush path never gets here. That is worth saying because +// the comment here used to claim otherwise, and the test named after the claim +// passed with the method deleted. +// +// What does need it is everything else the controller offers: SetWriteDeadline, +// SetReadDeadline, Hijack. A long-lived MCP stream is precisely the response +// that wants its write deadline pushed out, and without this method that call +// is ErrNotSupported. TestUnwrapReachesTheWriterBelow measures that against a +// real server, because a recorder supports no deadlines either way. func (w *cacheWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } diff --git a/mcphttp/cache_test.go b/mcphttp/cache_test.go index a7536ac704f876e6f66f1e25e60ada0eb95bca62..7191cc52b891715b6d70163c2aa8c7f3f9623429 100644 --- a/mcphttp/cache_test.go +++ b/mcphttp/cache_test.go @@ -2,6 +2,7 @@ package mcphttp_test import ( "bufio" + "io" "net/http" "net/http/httptest" "testing" @@ -87,15 +88,16 @@ func TestOnlyTheFirstCommitWrites(t *testing.T) { assert.Equal(t, "ab", rec.Body.String()) } -// TestUnwrapReachesTheUnderlyingFlusher is the test the Unwrap method exists -// for. +// TestFlushOnTheWrapperStreams covers the flush path, which cacheWriter answers +// itself rather than delegating. // -// cacheWriter embeds the http.ResponseWriter *interface*, so it promotes no -// Flush of its own; http.NewResponseController can only reach the real writer's -// through Unwrap. Delete the method and this fails with ErrNotSupported while -// every status-code and header test above stays green — which is precisely why -// it is written as a test and not as a comment. -func TestUnwrapReachesTheUnderlyingFlusher(t *testing.T) { +// It is deliberately NOT named for Unwrap. An earlier version of this test was +// named that and claimed to be what the Unwrap method exists for — and it passed +// with Unwrap deleted, because cacheWriter has a Flush method and +// http.NewResponseController prefers a method on the writer it is handed over +// one reached by unwrapping. It never got that far. See +// TestUnwrapReachesTheWriterBelow for the property that does need Unwrap. +func TestFlushOnTheWrapperStreams(t *testing.T) { var flushErr error h := mcphttp.PrivateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("event: message\n")) @@ -110,6 +112,35 @@ func TestUnwrapReachesTheUnderlyingFlusher(t *testing.T) { assert.Equal(t, wantCacheControl, rec.Header().Get("Cache-Control")) } +// TestUnwrapReachesTheWriterBelow is the test the Unwrap method actually earns. +// +// cacheWriter embeds the http.ResponseWriter *interface*, so it promotes nothing +// but the four methods that interface declares. Flush it answers itself; every +// other thing http.ResponseController offers — the deadlines, Hijack — can only +// be reached by unwrapping. A long-lived MCP stream is exactly the response that +// wants a write deadline pushed out, so this is not a hypothetical. +// +// It has to run against a real server: httptest.ResponseRecorder supports no +// deadlines at all, so a recorder would report ErrNotSupported whether Unwrap +// were there or not — the same vacuity the old test had. +func TestUnwrapReachesTheWriterBelow(t *testing.T) { + var deadlineErr error + srv := httptest.NewServer(mcphttp.PrivateCache(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + deadlineErr = http.NewResponseController(w).SetWriteDeadline(time.Now().Add(time.Minute)) + _, _ = w.Write([]byte("ok")) + }))) + t.Cleanup(srv.Close) + + resp, err := (&http.Client{Timeout: 5 * time.Second}).Get(srv.URL) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + _, _ = io.Copy(io.Discard, resp.Body) + + require.NoError(t, deadlineErr, + "without Unwrap the controller cannot reach the real writer and this is ErrNotSupported") +} + // TestAStreamReachesTheClientBeforeTheHandlerReturns is the same property // measured rather than asserted: over a real connection, with a real client, // bytes written and flushed inside the handler have to arrive while the handler