refactor(httputil): consolidate conditional-request serving

internal/httputil/conditional.go

@@ -0,0 +1,74 @@
+package httputil
+
+import (
+	"io"
+	"maps"
+	"net/http"
+	"strings"
+
+	"github.com/alecthomas/errors"
+)
+
+// ETagHeader is the HTTP header used to carry an object's entity tag.
+const ETagHeader = "ETag"
+
+// CheckConditionals evaluates RFC 7232 If-Match and If-None-Match precondition
+// headers against the stored ETag. It returns 0 when all preconditions pass,
+// otherwise the HTTP status code the caller should send: 412 Precondition
+// Failed for a failed If-Match, or 304 Not Modified for a satisfied
+// If-None-Match.
+func CheckConditionals(r *http.Request, etag string) int {
+	if ifMatch := r.Header.Get("If-Match"); ifMatch != "" {
+		if etag == "" || !etagListMatches(ifMatch, etag) {
+			return http.StatusPreconditionFailed
+		}
+	}
+	if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" {
+		if etag != "" && etagListMatches(ifNoneMatch, etag) {
+			return http.StatusNotModified
+		}
+	}
+	return 0
+}
+
+// etagListMatches reports whether etag matches an If-Match / If-None-Match
+// header value, which may be a comma-separated list of ETags or the "*"
+// wildcard. Stored ETags are always strong, so weak comparison is not required.
+func etagListMatches(headerValue, etag string) bool {
+	for candidate := range strings.SplitSeq(headerValue, ",") {
+		candidate = strings.TrimSpace(candidate)
+		if candidate == "*" || candidate == etag {
+			return true
+		}
+	}
+	return false
+}
+
+// ServeCacheHit serves a cache hit over HTTP. It copies the stored headers onto
+// the response, evaluates conditional request preconditions against the stored
+// ETag, and either short-circuits with a 304/412 status or streams the body.
+// The body is always closed.
+//
+// This consolidates the validator-aware serving path shared by handlers that
+// return a single cached object (e.g. the API and the generic caching handler).
+func ServeCacheHit(w http.ResponseWriter, r *http.Request, headers http.Header, body io.ReadCloser) error {
+	maps.Copy(w.Header(), headers)
+	if status := CheckConditionals(r, headers.Get(ETagHeader)); status != 0 {
+		w.WriteHeader(status)
+		return errors.WithStack(body.Close())
+	}
+	_, copyErr := io.Copy(w, body)
+	return errors.Wrap(errors.Join(copyErr, body.Close()), "serve cache hit")
+}
+
+// ServeCacheStat answers a metadata-only (HEAD) request from stored headers. It
+// copies the headers onto the response and writes the status determined by the
+// conditional request preconditions, defaulting to 200 OK.
+func ServeCacheStat(w http.ResponseWriter, r *http.Request, headers http.Header) {
+	maps.Copy(w.Header(), headers)
+	status := CheckConditionals(r, headers.Get(ETagHeader))
+	if status == 0 {
+		status = http.StatusOK
+	}
+	w.WriteHeader(status)
+}

internal/httputil/conditional_test.go

@@ -0,0 +1,151 @@
+package httputil_test
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/alecthomas/assert/v2"
+
+	"github.com/block/cachew/internal/httputil"
+)
+
+const testETag = `"abc123"`
+
+func cacheHeaders(extra ...[2]string) http.Header {
+	h := http.Header{}
+	h.Set(httputil.ETagHeader, testETag)
+	for _, kv := range extra {
+		h.Set(kv[0], kv[1])
+	}
+	return h
+}
+
+func newRequest(t *testing.T, ifMatch, ifNoneMatch string) *http.Request {
+	t.Helper()
+	r := httptest.NewRequest(http.MethodGet, "/", nil)
+	if ifMatch != "" {
+		r.Header.Set("If-Match", ifMatch)
+	}
+	if ifNoneMatch != "" {
+		r.Header.Set("If-None-Match", ifNoneMatch)
+	}
+	return r
+}
+
+func TestCheckConditionals(t *testing.T) {
+	tests := []struct {
+		name        string
+		ifMatch     string
+		ifNoneMatch string
+		etag        string
+		want        int
+	}{
+		{name: "NoConditions", etag: testETag, want: 0},
+		{name: "IfMatchExact", ifMatch: testETag, etag: testETag, want: 0},
+		{name: "IfMatchWildcard", ifMatch: "*", etag: testETag, want: 0},
+		{name: "IfMatchList", ifMatch: `"other", ` + testETag, etag: testETag, want: 0},
+		{name: "IfMatchMismatch", ifMatch: `"other"`, etag: testETag, want: http.StatusPreconditionFailed},
+		{name: "IfMatchNoETag", ifMatch: "*", etag: "", want: http.StatusPreconditionFailed},
+		{name: "IfNoneMatchExact", ifNoneMatch: testETag, etag: testETag, want: http.StatusNotModified},
+		{name: "IfNoneMatchWildcard", ifNoneMatch: "*", etag: testETag, want: http.StatusNotModified},
+		{name: "IfNoneMatchList", ifNoneMatch: `"other", ` + testETag, etag: testETag, want: http.StatusNotModified},
+		{name: "IfNoneMatchMismatch", ifNoneMatch: `"other"`, etag: testETag, want: 0},
+		{name: "IfNoneMatchNoETag", ifNoneMatch: "*", etag: "", want: 0},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := httputil.CheckConditionals(newRequest(t, tt.ifMatch, tt.ifNoneMatch), tt.etag)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+// trackingReader records whether Close was called, so tests can assert the body
+// is always released.
+type trackingReader struct {
+	io.Reader
+	closed bool
+}
+
+func (t *trackingReader) Close() error {
+	t.closed = true
+	return nil
+}
+
+func TestServeCacheHit(t *testing.T) {
+	t.Run("StreamsBodyWhenFresh", func(t *testing.T) {
+		body := &trackingReader{Reader: strings.NewReader("payload")}
+		headers := cacheHeaders([2]string{"Content-Type", "text/plain"})
+		w := httptest.NewRecorder()
+
+		err := httputil.ServeCacheHit(w, newRequest(t, "", ""), headers, body)
+		assert.NoError(t, err)
+		assert.True(t, body.closed)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		assert.Equal(t, testETag, resp.Header.Get("ETag"))
+		assert.Equal(t, "text/plain", resp.Header.Get("Content-Type"))
+		data, _ := io.ReadAll(resp.Body)
+		assert.Equal(t, "payload", string(data))
+	})
+
+	t.Run("NotModifiedSkipsBody", func(t *testing.T) {
+		body := &trackingReader{Reader: strings.NewReader("payload")}
+		headers := cacheHeaders()
+		w := httptest.NewRecorder()
+
+		err := httputil.ServeCacheHit(w, newRequest(t, "", testETag), headers, body)
+		assert.NoError(t, err)
+		assert.True(t, body.closed)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusNotModified, resp.StatusCode)
+		data, _ := io.ReadAll(resp.Body)
+		assert.Equal(t, "", string(data))
+	})
+
+	t.Run("PreconditionFailedSkipsBody", func(t *testing.T) {
+		body := &trackingReader{Reader: strings.NewReader("payload")}
+		headers := cacheHeaders()
+		w := httptest.NewRecorder()
+
+		err := httputil.ServeCacheHit(w, newRequest(t, `"other"`, ""), headers, body)
+		assert.NoError(t, err)
+		assert.True(t, body.closed)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusPreconditionFailed, resp.StatusCode)
+	})
+}
+
+func TestServeCacheStat(t *testing.T) {
+	t.Run("OKWhenFresh", func(t *testing.T) {
+		headers := cacheHeaders()
+		w := httptest.NewRecorder()
+
+		httputil.ServeCacheStat(w, newRequest(t, "", ""), headers)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		assert.Equal(t, testETag, resp.Header.Get("ETag"))
+	})
+
+	t.Run("NotModified", func(t *testing.T) {
+		headers := cacheHeaders()
+		w := httptest.NewRecorder()
+
+		httputil.ServeCacheStat(w, newRequest(t, "", testETag), headers)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusNotModified, resp.StatusCode)
+	})
+}

internal/strategy/apiv1.go

@@ -5,10 +5,8 @@ import (
 	"encoding/json"
 	"io"
 	"log/slog"
-	"maps"
 	"net/http"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/alecthomas/errors"
@@ -69,12 +67,7 @@ func (d *APIV1) statObject(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	maps.Copy(w.Header(), headers)
-	if status := CheckConditionals(r, headers.Get("ETag")); status != 0 {
-		w.WriteHeader(status)
-		return
-	}
-	w.WriteHeader(http.StatusOK)
+	httputil.ServeCacheStat(w, r, headers)
 }
 
 func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) {
@@ -100,19 +93,8 @@ func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	maps.Copy(w.Header(), headers)
-	if status := CheckConditionals(r, headers.Get("ETag")); status != 0 {
-		_ = cr.Close()
-		w.WriteHeader(status)
-		return
-	}
-
-	_, err = io.Copy(w, cr)
-	if err != nil {
-		d.logger.Error("Failed to copy cache object to response", "error", err, "key", key)
-	}
-	if cerr := cr.Close(); cerr != nil {
-		d.logger.Error("Failed to close cache reader", "error", cerr, "key", key)
+	if err := httputil.ServeCacheHit(w, r, headers, cr); err != nil {
+		d.logger.Error("Failed to serve cache object", "error", err, "key", key)
 	}
 }
 
@@ -218,34 +200,3 @@ func (d *APIV1) httpError(w http.ResponseWriter, code int, err error, message st
 	d.logger.Error(message, args...)
 	http.Error(w, message, code)
 }
-
-// CheckConditionals evaluates If-Match and If-None-Match precondition headers
-// against the stored ETag for GET/HEAD requests. Returns 0 if all
-// preconditions pass, otherwise the HTTP status code to send (304 or 412).
-func CheckConditionals(r *http.Request, etag string) int {
-	if ifMatch := r.Header.Get("If-Match"); ifMatch != "" {
-		if etag == "" || !etagListMatches(ifMatch, etag) {
-			return http.StatusPreconditionFailed
-		}
-	}
-	if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" {
-		if etag != "" && etagListMatches(ifNoneMatch, etag) {
-			return http.StatusNotModified
-		}
-	}
-	return 0
-}
-
-// etagListMatches reports whether etag matches an RFC 7232 If-Match /
-// If-None-Match header value, which may be a comma-separated list of ETags or
-// the "*" wildcard. Stored ETags are always strong, so weak comparison is not
-// required.
-func etagListMatches(headerValue, etag string) bool {
-	for candidate := range strings.SplitSeq(headerValue, ",") {
-		candidate = strings.TrimSpace(candidate)
-		if candidate == "*" || candidate == etag {
-			return true
-		}
-	}
-	return false
-}

internal/strategy/git/snapshot.go

@@ -20,9 +20,9 @@ import (
 
 	"github.com/block/cachew/internal/cache"
 	"github.com/block/cachew/internal/gitclone"
+	"github.com/block/cachew/internal/httputil"
 	"github.com/block/cachew/internal/logging"
 	"github.com/block/cachew/internal/snapshot"
-	"github.com/block/cachew/internal/strategy"
 )
 
 const lfsFetchTimeout = 25 * time.Minute
@@ -367,7 +367,7 @@ func (s *Strategy) serveSnapshotHead(ctx context.Context, w http.ResponseWriter,
 	}
 
 	status := http.StatusOK
-	if conditional := strategy.CheckConditionals(r, headers.Get(cache.ETagKey)); conditional != 0 {
+	if conditional := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); conditional != 0 {
 		status = conditional
 	}
 	w.WriteHeader(status)
@@ -557,7 +557,7 @@ func (s *Strategy) serveSnapshotWithBundle(ctx context.Context, w http.ResponseW
 	// avoid streaming the full snapshot when the client already has it.
 	var n int64
 	var err error
-	if status := strategy.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 {
+	if status := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 {
 		w.WriteHeader(status)
 	} else {
 		n, err = serveReaderFast(w, r, reader)

internal/strategy/handler/handler.go

@@ -171,12 +171,7 @@ func (h *Handler) serveCached(w http.ResponseWriter, r *http.Request, key cache.
 	}
 
 	logging.FromContext(r.Context()).DebugContext(r.Context(), "Cache hit")
-	defer cr.Close()
-	maps.Copy(w.Header(), headers)
-	if _, err := io.Copy(w, cr); err != nil {
-		return true, errors.Wrap(err, "stream from cache")
-	}
-	return true, nil
+	return true, errors.WithStack(httputil.ServeCacheHit(w, r, headers, cr))
 }
 
 func (h *Handler) fetchAndCache(w http.ResponseWriter, r *http.Request, key cache.Key) error {