From e30db2daa9785c55491fa482c356491d0134af4f Mon Sep 17 00:00:00 2001 From: Bart Venter Date: Sun, 29 Jun 2025 10:32:01 +0000 Subject: [PATCH 1/4] chore(testutil): Improve error formatting --- internal/testutil/testutil.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index a2b19c4..78dc65a 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -61,7 +61,7 @@ func AssertEqual[T cmp.Ordered](t *testing.T, expected, actual T, msgAndArgs ... return assert( t, got == 0, - "assertEqual failed: expected %v, got %v, %s", + "assertEqual failed: expected %q, got %q, %s", expected, actual, msgAndArgs, From a18013603c87d97e72b824f4eb2abc38a9dc3997 Mon Sep 17 00:00:00 2001 From: Bart Venter Date: Sun, 29 Jun 2025 10:40:56 +0000 Subject: [PATCH 2/4] fix: use sentinel hash for empty Vary in cache key This commit ensures a sentinel value is appended to the URL key when no Vary headers are present. This prevents collisions between the base URL key and the vary key, avoiding self-referential cache entries. --- internal/normalization.go | 4 +++- internal/normalization_test.go | 11 ++--------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/internal/normalization.go b/internal/normalization.go index cae22a8..d093018 100644 --- a/internal/normalization.go +++ b/internal/normalization.go @@ -324,9 +324,11 @@ func (f VaryKeyerFunc) VaryKey(urlKey string, varyHeaders map[string]string) str func NewVaryKeyer() VaryKeyer { return VaryKeyerFunc(makeVaryKey) } +const noVaryHash = "0" + func makeVaryKey(urlKey string, varyHeaders map[string]string) string { if len(varyHeaders) == 0 { - return urlKey // No vary headers means no variation + return urlKey + "#" + noVaryHash // No Vary headers, so no variations } varyHash := makeVaryHash(varyHeaders) return urlKey + "#" + strconv.FormatUint(varyHash, 10) diff --git a/internal/normalization_test.go b/internal/normalization_test.go index 7e1a3dc..d5bf188 100644 --- a/internal/normalization_test.go +++ b/internal/normalization_test.go @@ -101,19 +101,12 @@ func Test_makeVaryKey(t *testing.T) { wantPrefix string wantHash func() string }{ - { - name: "no vary headers (empty map)", - urlKey: urlKey, - vary: map[string]string{}, - wantPrefix: urlKey, - wantHash: func() string { return "" }, - }, { name: "no vary headers (nil map)", urlKey: urlKey, vary: nil, - wantPrefix: urlKey, - wantHash: func() string { return "" }, + wantPrefix: urlKey + "#", + wantHash: func() string { return noVaryHash }, }, { name: "single header", From c810fde74f8bc4d211dbceb1550b88d476a7539a Mon Sep 17 00:00:00 2001 From: Bart Venter Date: Sun, 29 Jun 2025 10:42:56 +0000 Subject: [PATCH 3/4] chore: Improve logging for cache entry retrieval errors --- internal/responsecache.go | 48 +++++++++++++++++++++++++++++++++++---- roundtripper.go | 10 ++++---- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/internal/responsecache.go b/internal/responsecache.go index 2953ab4..b513a47 100644 --- a/internal/responsecache.go +++ b/internal/responsecache.go @@ -17,6 +17,7 @@ package internal import ( "encoding/json" "fmt" + "log/slog" "net/http" "github.com/bartventer/httpcache/store/driver" @@ -46,6 +47,29 @@ func NewResponseCache(cache Cache) *responseCache { var _ ResponseCache = (*responseCache)(nil) +type CacheError struct { + Op string + Message string + Err error +} + +func (c *CacheError) Error() string { return fmt.Sprintf("%s: %s: %v", c.Op, c.Message, c.Err) } +func (c *CacheError) Unwrap() error { return c.Err } + +func (c *CacheError) LogValue() slog.Value { + return slog.GroupValue( + slog.String("op", c.Op), + slog.String("message", c.Message), + slog.Any("error", c.Err), + ) +} + +func newCacheError(err error, op, message string) *CacheError { + return &CacheError{Op: op, Message: message, Err: err} +} + +var _ slog.LogValuer = (*CacheError)(nil) + func (r *responseCache) Get(responseKey string, req *http.Request) (*Response, error) { data, err := r.cache.Get(responseKey) if err != nil { @@ -53,7 +77,11 @@ func (r *responseCache) Get(responseKey string, req *http.Request) (*Response, e } entry, err := ParseResponse(data, req) if err != nil { - return nil, fmt.Errorf("failed to unmarshal cached entry: %w", err) + return nil, newCacheError( + err, + "Get", + fmt.Sprintf("failed to unmarshal cached entry for key %q", responseKey), + ) } return entry, nil } @@ -61,7 +89,11 @@ func (r *responseCache) Get(responseKey string, req *http.Request) (*Response, e func (r *responseCache) Set(responseKey string, entry *Response) error { data, err := entry.MarshalBinary() if err != nil { - return fmt.Errorf("failed to marshal entry: %w", err) + return newCacheError( + err, + "Set", + fmt.Sprintf("failed to marshal entry for key %q", responseKey), + ) } return r.cache.Set(responseKey, data) } @@ -77,7 +109,11 @@ func (r *responseCache) GetRefs(urlKey string) (ResponseRefs, error) { } var refs ResponseRefs if unmarshalErr := json.Unmarshal(data, &refs); unmarshalErr != nil { - return nil, fmt.Errorf("failed to unmarshal cached entries: %w", unmarshalErr) + return nil, newCacheError( + unmarshalErr, + "GetRefs", + fmt.Sprintf("failed to unmarshal cached refs for key %q", urlKey), + ) } return refs, nil } @@ -85,7 +121,11 @@ func (r *responseCache) GetRefs(urlKey string) (ResponseRefs, error) { func (r *responseCache) SetRefs(urlKey string, refs ResponseRefs) error { data, err := json.Marshal(refs) if err != nil { - return fmt.Errorf("failed to marshal headers: %w", err) + return newCacheError( + err, + "SetRefs", + fmt.Sprintf("failed to marshal refs for key %q", urlKey), + ) } return r.cache.Set(urlKey, data) } diff --git a/roundtripper.go b/roundtripper.go index e5d00d6..7d6c66b 100644 --- a/roundtripper.go +++ b/roundtripper.go @@ -54,6 +54,7 @@ import ( "iter" "log/slog" "net/http" + "strings" "time" "github.com/bartventer/httpcache/internal" @@ -224,13 +225,14 @@ func (r *transport) RoundTrip(req *http.Request) (*http.Response, error) { entry, err := r.cache.Get(refs[refIndex].ResponseID, req) if err != nil || entry == nil { r.logger.Warn( - "Cache reference found but entry missing or unreadable; possible cache corruption or concurrent eviction. Falling back to cache miss.", + "Cache entry missing or unreadable. Possible corruption or eviction.", slog.String("url", req.URL.String()), slog.String("method", req.Method), - slog.String("cacheKey", urlKey), - slog.Int("refIndex", refIndex), + slog.String("drivers", strings.Join(store.Drivers(), ", ")), + slog.String("cache_key", urlKey), + slog.Int("ref_index", refIndex), slog.String("vary", refs[refIndex].Vary), - slog.String("responseID", refs[refIndex].ResponseID), + slog.String("response_id", refs[refIndex].ResponseID), slog.Any("error", err), ) return r.handleCacheMiss(req, urlKey, refs, refIndex) From 2c86d11a8a54877b9f10cd51dcf79e3dc2a7aeb0 Mon Sep 17 00:00:00 2001 From: Bart Venter Date: Sun, 29 Jun 2025 10:44:05 +0000 Subject: [PATCH 4/4] refactor: Rename internal header constants for consistency --- internal/header.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/header.go b/internal/header.go index 2cb54f7..f5b5cd9 100644 --- a/internal/header.go +++ b/internal/header.go @@ -19,8 +19,8 @@ import ( ) const ( - CacheStatusHeader = "X-Httpcache-Status" - CacheStatusHeaderLegacy = "X-From-Cache" // Deprecated: use [CacheStatusHeader] instead + CacheStatusHeader = "X-Httpcache-Status" + FromCacheHeader = "X-From-Cache" // Deprecated: use [CacheStatusHeader] instead ) type CacheStatus struct { @@ -37,14 +37,19 @@ type CacheStatus struct { func (s CacheStatus) ApplyTo(header http.Header) { header.Set(CacheStatusHeader, s.Value) if s.Legacy != "" { - header.Set(CacheStatusHeaderLegacy, s.Legacy) + header.Set(FromCacheHeader, s.Legacy) } } +const ( + FromCache = "1" + NotFromCache = "" +) + var ( - CacheStatusHit = CacheStatus{"HIT", "1"} // served from cache - CacheStatusMiss = CacheStatus{"MISS", ""} // served from origin - CacheStatusStale = CacheStatus{"STALE", "1"} // served from cache but stale - CacheStatusRevalidated = CacheStatus{"REVALIDATED", "1"} // revalidated with origin server - CacheStatusBypass = CacheStatus{"BYPASS", ""} // cache bypassed + CacheStatusHit = CacheStatus{"HIT", FromCache} // served from cache + CacheStatusMiss = CacheStatus{"MISS", NotFromCache} // served from origin + CacheStatusStale = CacheStatus{"STALE", FromCache} // served from cache but stale + CacheStatusRevalidated = CacheStatus{"REVALIDATED", FromCache} // revalidated with origin server + CacheStatusBypass = CacheStatus{"BYPASS", NotFromCache} // cache bypassed )