~bigbes/sr-ht-ecore

3bd158fbb23241df31897f9ba47afac832ee2e1d — Eugene Blikh 2 days ago 00d7582
mcphttp: pin Flush with a flush that precedes any write

Deleting the Flush method left every test in this package green. All of
them wrote before flushing, which commits the headers through the Write
hook and leaves the flush path unmeasured — the second vacuous test in
this file, after the Unwrap one.

The case that needs Flush is a handler that flushes first, as an SSE
handler opening a stream does. It has to run against a real server:
httptest.ResponseRecorder's Header() hands back the live map, so a
recorder reports the headers as set whenever they were set, and passes
for exactly this bug. Found by an agent that measured it in a sibling
service's copy of this code.
1 files changed, 38 insertions(+), 0 deletions(-)

M mcphttp/cache_test.go
M mcphttp/cache_test.go => mcphttp/cache_test.go +38 -0
@@ 88,6 88,44 @@ func TestOnlyTheFirstCommitWrites(t *testing.T) {
	assert.Equal(t, "ab", rec.Body.String())
}

// TestAFlushBeforeAnyWriteStillCarriesTheHeaders is what the Flush method earns.
//
// A handler that flushes before writing anything commits the response through
// the flush, not through Write — so the Write hook never runs, and without a
// Flush of its own the wrapper is bypassed entirely and the response leaves with
// no Cache-Control and no Vary. Every other test here writes first, which sets
// the headers on the way in and leaves the flush path unmeasured: deleting Flush
// kept all of them green.
//
// It has to run against a real server. httptest.ResponseRecorder's Header()
// hands back the live map, so a recorder reports the headers as set no matter
// when — or whether — the wrapper committed them, and reports a pass for exactly
// this bug.
func TestAFlushBeforeAnyWriteStillCarriesTheHeaders(t *testing.T) {
	release := make(chan struct{})
	srv := httptest.NewServer(mcphttp.PrivateCache(http.HandlerFunc(
		func(w http.ResponseWriter, _ *http.Request) {
			w.Header().Set("Content-Type", "text/event-stream")
			if err := http.NewResponseController(w).Flush(); err != nil {
				t.Errorf("flush: %v", err)
			}
			<-release
			_, _ = w.Write([]byte(": keepalive\n\n"))
		})))
	t.Cleanup(func() {
		close(release)
		srv.Close()
	})

	resp, err := (&http.Client{Timeout: 5 * time.Second}).Get(srv.URL)
	require.NoError(t, err)
	t.Cleanup(func() { _ = resp.Body.Close() })

	assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control"),
		"the response committed through Flush, so only Flush could have set this")
	assert.Equal(t, wantVary, resp.Header.Get("Vary"))
}

// TestFlushOnTheWrapperStreams covers the flush path, which cacheWriter answers
// itself rather than delegating.
//