fix(pgs): httpcache rw.Write needs to report the num of bytes

Commit `034d3f3`

Eric Bower · 2026-04-19 10:50:47 -0400 EDT

parent 588159a

fix(pgs): httpcache rw.Write needs to report the num of bytes

3 files changed, +121, -13

M pkg/httpcache/cache_test.go

M pkg/httpcache/rw.go

M pkg/httpcache/serve.go

M pkg/httpcache/cache_test.go

+115, -0

......@@ -799,6 +799,121 @@ func TestCache304NotModifiedMerge(t *testing.T) {
	}
}

func TestCacheUpstreamResponseBody(t *testing.T) {
	expectedBody := strings.Repeat("hello world! ", 1000)
	mux := http.NewServeMux()
	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("content-length", strconv.Itoa(len(expectedBody)))
		w.WriteHeader(200)
		_, _ = w.Write([]byte(expectedBody))
	})

	logger := slog.Default()
	handler := NewHttpCache(logger, mux)
	tc := NewTestContext(t, handler)
	req, _ := http.NewRequest("GET", tc.cachedServer.URL+"/test", nil)

	// first request goes to upstream
	resp1, _ := tc.Do(req)
	if resp1.StatusCode != http.StatusOK {
		t.Fatalf("expected 200, got %d", resp1.StatusCode)
	}
	body1, _ := readBody(resp1)
	if body1 != expectedBody {
		t.Errorf("upstream body mismatch: got %d bytes, want %d bytes", len(body1), len(expectedBody))
	}

	// second request served from cache
	resp2, _ := tc.Do(req)
	if resp2.StatusCode != http.StatusOK {
		t.Fatalf("expected 200, got %d", resp2.StatusCode)
	}
	body2, _ := readBody(resp2)
	if body2 != expectedBody {
		t.Errorf("cached body mismatch: got %d bytes, want %d bytes", len(body2), len(expectedBody))
	}
}

func TestCacheUpstreamStatusCode(t *testing.T) {
	mux := http.NewServeMux()
	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		w.WriteHeader(201)
		_, _ = w.Write([]byte("created"))
	})

	logger := slog.Default()
	handler := NewHttpCache(logger, mux)
	tc := NewTestContext(t, handler)
	req, _ := http.NewRequest("GET", tc.cachedServer.URL+"/test", nil)

	resp, _ := tc.Do(req)
	if resp.StatusCode != 201 {
		t.Errorf("expected 201, got %d", resp.StatusCode)
	}
	body, _ := readBody(resp)
	if body != "created" {
		t.Errorf("expected body 'created', got %q", body)
	}
}

// RFC 9110 15.4.5: 304 responses MUST NOT contain a body.
// Even if the upstream handler writes body bytes with a 304,
// the cache layer must strip them before sending to the client.
func TestCache304NoBody(t *testing.T) {
	mux := http.NewServeMux()
	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		if r.Header.Get("If-None-Match") == "\"abc\"" {
			w.WriteHeader(http.StatusNotModified)
			// Misbehaving upstream writes body alongside 304
			_, _ = w.Write([]byte("should not appear"))
			return
		}
		w.Header().Set("etag", "\"abc\"")
		w.Header().Set("cache-control", "max-age=60, must-revalidate")
		w.WriteHeader(200)
		_, _ = w.Write([]byte("original body"))
	})

	logger := slog.Default()
	handler := NewHttpCache(logger, mux)
	tc := NewTestContext(t, handler)

	req, _ := http.NewRequest("GET", tc.cachedServer.URL+"/test", nil)

	// Populate cache with a stale must-revalidate entry so revalidation is triggered
	cacheKey := handler.GetCacheKey(req)
	cv := testCacheValue(250 * time.Second)
	cv.Header["ETag"] = []string{"\"abc\""}
	cv.Header["Cache-Control"] = []string{"max-age=60, must-revalidate"}
	cv.Body = []byte("original body")
	cacheData, _ := json.Marshal(cv)
	handler.Cache.Add(cacheKey, cacheData)

	// Trigger revalidation — upstream returns 304 with a spurious body
	resp, _ := tc.Do(req)
	if resp.StatusCode != http.StatusNotModified {
		t.Fatalf("expected 304, got %d", resp.StatusCode)
	}
	body, _ := readBody(resp)
	if body != "" {
		t.Errorf("expected empty body for 304 response, got %q", body)
	}
}

func readBody(resp *http.Response) (string, error) {
	defer resp.Body.Close() //nolint:errcheck
	buf := make([]byte, 0, 64*1024)
	tmp := make([]byte, 4096)
	for {
		n, err := resp.Body.Read(tmp)
		buf = append(buf, tmp[:n]...)
		if err != nil {
			break
		}
	}
	return string(buf), nil
}

func TestCacheAgeTtl(t *testing.T) {
	mux := http.NewServeMux()
	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {

M pkg/httpcache/rw.go

+6, -3

......@@ -15,11 +15,9 @@ func (rw *responseWriter) WriteHeader(code int) {
	rw.statusCode = code
}

// TODO: is there a way to preserve streaming to the base response writer while being able to set headers?
func (rw *responseWriter) Write(data []byte) (int, error) {
	rw.body = append(rw.body, data...)
	// return rw.ResponseWriter.Write(data)
	return 0, nil
	return len(data), nil
}

// Body returns the captured response body.

......@@ -37,6 +35,11 @@ func (rw *responseWriter) StatusCode() int {

func (rw *responseWriter) Send() {
	rw.ResponseWriter.WriteHeader(rw.StatusCode())
	// RFC 9110 15.4.5: 304 responses MUST NOT contain a body.
	if rw.StatusCode() == http.StatusNotModified {
		return
	}
	_, _ = rw.ResponseWriter.Write(rw.body)
}

func (rw *responseWriter) ToCacheValue() *CacheValue {

M pkg/httpcache/serve.go

+0, -10

......@@ -147,16 +147,6 @@ func (c *HttpCache) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	}

	wrapped.Send()
	// RFC 9110 15.4.5: 304 responses MUST NOT contain a body.
	// Skip writing body for 304 responses even if upstream wrote one.
	var total int
	if wrapped.StatusCode() != http.StatusNotModified {
		total, err = wrapped.ResponseWriter.Write(wrapped.Body())
	}
	log.Info("response writer", "bytes_written", total)
	if err != nil {
		log.Error("response writer write", "err", err)
	}
}

// isForbiddenHeader checks if a header should not be stored/served per RFC 9111 Section 3.1

...	...		@@ -799,6 +799,121 @@ func TestCache304NotModifiedMerge(t *testing.T) {
799	799		}
800	800		}
801	801
	802	+	func TestCacheUpstreamResponseBody(t *testing.T) {
	803	+	expectedBody := strings.Repeat("hello world! ", 1000)
	804	+	mux := http.NewServeMux()
	805	+	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	806	+	w.Header().Set("content-length", strconv.Itoa(len(expectedBody)))
	807	+	w.WriteHeader(200)
	808	+	_, _ = w.Write([]byte(expectedBody))
	809	+	})
	810	+
	811	+	logger := slog.Default()
	812	+	handler := NewHttpCache(logger, mux)
	813	+	tc := NewTestContext(t, handler)
	814	+	req, _ := http.NewRequest("GET", tc.cachedServer.URL+"/test", nil)
	815	+
	816	+	// first request goes to upstream
	817	+	resp1, _ := tc.Do(req)
	818	+	if resp1.StatusCode != http.StatusOK {
	819	+	t.Fatalf("expected 200, got %d", resp1.StatusCode)
	820	+	}
	821	+	body1, _ := readBody(resp1)
	822	+	if body1 != expectedBody {
	823	+	t.Errorf("upstream body mismatch: got %d bytes, want %d bytes", len(body1), len(expectedBody))
	824	+	}
	825	+
	826	+	// second request served from cache
	827	+	resp2, _ := tc.Do(req)
	828	+	if resp2.StatusCode != http.StatusOK {
	829	+	t.Fatalf("expected 200, got %d", resp2.StatusCode)
	830	+	}
	831	+	body2, _ := readBody(resp2)
	832	+	if body2 != expectedBody {
	833	+	t.Errorf("cached body mismatch: got %d bytes, want %d bytes", len(body2), len(expectedBody))
	834	+	}
	835	+	}
	836	+
	837	+	func TestCacheUpstreamStatusCode(t *testing.T) {
	838	+	mux := http.NewServeMux()
	839	+	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	840	+	w.WriteHeader(201)
	841	+	_, _ = w.Write([]byte("created"))
	842	+	})
	843	+
	844	+	logger := slog.Default()
	845	+	handler := NewHttpCache(logger, mux)
	846	+	tc := NewTestContext(t, handler)
	847	+	req, _ := http.NewRequest("GET", tc.cachedServer.URL+"/test", nil)
	848	+
	849	+	resp, _ := tc.Do(req)
	850	+	if resp.StatusCode != 201 {
	851	+	t.Errorf("expected 201, got %d", resp.StatusCode)
	852	+	}
	853	+	body, _ := readBody(resp)
	854	+	if body != "created" {
	855	+	t.Errorf("expected body 'created', got %q", body)
	856	+	}
	857	+	}
	858	+
	859	+	// RFC 9110 15.4.5: 304 responses MUST NOT contain a body.
	860	+	// Even if the upstream handler writes body bytes with a 304,
	861	+	// the cache layer must strip them before sending to the client.
	862	+	func TestCache304NoBody(t *testing.T) {
	863	+	mux := http.NewServeMux()
	864	+	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	865	+	if r.Header.Get("If-None-Match") == "\"abc\"" {
	866	+	w.WriteHeader(http.StatusNotModified)
	867	+	// Misbehaving upstream writes body alongside 304
	868	+	_, _ = w.Write([]byte("should not appear"))
	869	+	return
	870	+	}
	871	+	w.Header().Set("etag", "\"abc\"")
	872	+	w.Header().Set("cache-control", "max-age=60, must-revalidate")
	873	+	w.WriteHeader(200)
	874	+	_, _ = w.Write([]byte("original body"))
	875	+	})
	876	+
	877	+	logger := slog.Default()
	878	+	handler := NewHttpCache(logger, mux)
	879	+	tc := NewTestContext(t, handler)
	880	+
	881	+	req, _ := http.NewRequest("GET", tc.cachedServer.URL+"/test", nil)
	882	+
	883	+	// Populate cache with a stale must-revalidate entry so revalidation is triggered
	884	+	cacheKey := handler.GetCacheKey(req)
	885	+	cv := testCacheValue(250 * time.Second)
	886	+	cv.Header["ETag"] = []string{"\"abc\""}
	887	+	cv.Header["Cache-Control"] = []string{"max-age=60, must-revalidate"}
	888	+	cv.Body = []byte("original body")
	889	+	cacheData, _ := json.Marshal(cv)
	890	+	handler.Cache.Add(cacheKey, cacheData)
	891	+
	892	+	// Trigger revalidation — upstream returns 304 with a spurious body
	893	+	resp, _ := tc.Do(req)
	894	+	if resp.StatusCode != http.StatusNotModified {
	895	+	t.Fatalf("expected 304, got %d", resp.StatusCode)
	896	+	}
	897	+	body, _ := readBody(resp)
	898	+	if body != "" {
	899	+	t.Errorf("expected empty body for 304 response, got %q", body)
	900	+	}
	901	+	}
	902	+
	903	+	func readBody(resp *http.Response) (string, error) {
	904	+	defer resp.Body.Close() //nolint:errcheck
	905	+	buf := make([]byte, 0, 64*1024)
	906	+	tmp := make([]byte, 4096)
	907	+	for {
	908	+	n, err := resp.Body.Read(tmp)
	909	+	buf = append(buf, tmp[:n]...)
	910	+	if err != nil {
	911	+	break
	912	+	}
	913	+	}
	914	+	return string(buf), nil
	915	+	}
	916	+
802	917		func TestCacheAgeTtl(t *testing.T) {
803	918		mux := http.NewServeMux()
804	919		mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {

...	...		@@ -147,16 +147,6 @@ func (c HttpCache) ServeHTTP(w http.ResponseWriter, r http.Request) {
147	147		}
148	148
149	149		wrapped.Send()
150		-	// RFC 9110 15.4.5: 304 responses MUST NOT contain a body.
151		-	// Skip writing body for 304 responses even if upstream wrote one.
152		-	var total int
153		-	if wrapped.StatusCode() != http.StatusNotModified {
154		-	total, err = wrapped.ResponseWriter.Write(wrapped.Body())
155		-	}
156		-	log.Info("response writer", "bytes_written", total)
157		-	if err != nil {
158		-	log.Error("response writer write", "err", err)
159		-	}
160	150		}
161	151
162	152		// isForbiddenHeader checks if a header should not be stored/served per RFC 9111 Section 3.1

...	...		@@ -15,11 +15,9 @@ func (rw *responseWriter) WriteHeader(code int) {
15	15		rw.statusCode = code
16	16		}
17	17
18		-	// TODO: is there a way to preserve streaming to the base response writer while being able to set headers?
19	18		func (rw *responseWriter) Write(data []byte) (int, error) {
20	19		rw.body = append(rw.body, data...)
21		-	// return rw.ResponseWriter.Write(data)
22		-	return 0, nil
	20	+	return len(data), nil
23	21		}
24	22
25	23		// Body returns the captured response body.

...	...		@@ -37,6 +35,11 @@ func (rw *responseWriter) StatusCode() int {
37	35
38	36		func (rw *responseWriter) Send() {
39	37		rw.ResponseWriter.WriteHeader(rw.StatusCode())
	38	+	// RFC 9110 15.4.5: 304 responses MUST NOT contain a body.
	39	+	if rw.StatusCode() == http.StatusNotModified {
	40	+	return
	41	+	}
	42	+	_, _ = rw.ResponseWriter.Write(rw.body)
40	43		}
41	44
42	45		func (rw responseWriter) ToCacheValue() CacheValue {

Commit 034d3f3

Commit `034d3f3`