From ca9eaf20b4e2ac3c365c3fd4f99aec015b05c49c Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 08:48:23 +0000 Subject: [PATCH 01/13] test(proxy): add cache body length regression coverage Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 359 ++++++++++++++++++++++++++++++++ 1 file changed, 359 insertions(+) create mode 100644 proxy/cache_body_length_test.go diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go new file mode 100644 index 00000000..16d65d7d --- /dev/null +++ b/proxy/cache_body_length_test.go @@ -0,0 +1,359 @@ +package proxy + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "strconv" + "sync/atomic" + "testing" + + dto "github.com/prometheus/client_model/go" + cacheclient "github.com/tigrisdata/ocache/client" + "github.com/tigrisdata/tag/cache" + "github.com/tigrisdata/tag/config" + "github.com/tigrisdata/tag/metrics" +) + +func newWholeObjectCacheService(t *testing.T, objectBody, cacheBody []byte) (*Service, *cache.CachedObjectMeta) { + t.Helper() + + cfg := config.NewDefault() + cfg.Cache.SetBlockCachingEnabled(false) + store := cache.NewCacheWithClient(cacheclient.NewMemoryCache(), &cfg.Cache) + meta := &cache.CachedObjectMeta{ + Bucket: "bucket", + Key: "key", + ETag: `"etag"`, + ContentType: "text/plain", + ContentLength: int64(len(objectBody)), + StatusCode: http.StatusOK, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, cacheBody, 0); err != nil { + t.Fatalf("seed whole-object cache: %v", err) + } + return NewService(&mockForwarder{}, store, cfg), meta +} + +func requestCountForOperation(t *testing.T, operation, status, source string) float64 { + t.Helper() + + var metric dto.Metric + if err := metrics.RequestsTotal.WithLabelValues(operation, status, "unknown", source).Write(&metric); err != nil { + t.Fatalf("read %s/%s request count: %v", operation, status, err) + } + return metric.GetCounter().GetValue() +} + +func newTieredShortBodyService(t *testing.T, objectBody, cacheBody []byte) (*Service, *cache.Cache, *atomic.Int64, *cache.CachedObjectMeta) { + t.Helper() + + forwarder, forwards, _ := tieredMock() + svc, store := newTieredTestService(forwarder, int64(len(objectBody)+1)) + if w := tieredDo(t, svc, http.MethodPut, "/bucket/key", string(objectBody), nil); w.Code != http.StatusOK { + t.Fatalf("seed tiered PUT status = %d, want %d", w.Code, http.StatusOK) + } + meta, found, err := store.GetMeta(context.Background(), "bucket", "key") + if err != nil || !found || meta == nil || meta.BodyRef == "" { + t.Fatalf("seed tiered metadata = %+v, found=%v, err=%v", meta, found, err) + } + if err := store.PutBodyStream(context.Background(), "bucket", "key", meta.BodyRef, bytes.NewReader(cacheBody), 0); err != nil { + t.Fatalf("seed tiered cache body: %v", err) + } + return svc, store, forwards, meta +} + +func TestHandleGetObject_CacheBodyLength(t *testing.T) { + smallBody := []byte("small cache body") + largeBody := bytes.Repeat([]byte("cached body"), 8192) + proxyCases := []struct { + name string + objectBody []byte + cacheBody []byte + wantBody []byte + wantCacheStatus string + wantContentLength string + wantUpstreamCalls int32 + wantSource string + wantRequestStatus string + }{ + { + name: "short-small", + objectBody: smallBody, + cacheBody: smallBody[:5], + wantBody: smallBody, + wantCacheStatus: XCacheMiss, + wantContentLength: strconv.Itoa(len(smallBody)), + wantUpstreamCalls: 1, + wantSource: metrics.SourceUpstream, + wantRequestStatus: "success", + }, + { + name: "short-large", + objectBody: largeBody, + cacheBody: largeBody[:32768], + wantBody: largeBody[:32768], + wantCacheStatus: XCacheHit, + wantContentLength: strconv.Itoa(len(largeBody)), + wantSource: metrics.SourceLocal, + wantRequestStatus: "error", + }, + { + name: "exact-small", + objectBody: smallBody, + cacheBody: smallBody, + wantBody: smallBody, + wantCacheStatus: XCacheHit, + wantContentLength: strconv.Itoa(len(smallBody)), + wantSource: metrics.SourceLocal, + wantRequestStatus: "success", + }, + { + name: "exact-large", + objectBody: largeBody, + cacheBody: largeBody, + wantBody: largeBody, + wantCacheStatus: XCacheHit, + wantContentLength: strconv.Itoa(len(largeBody)), + wantSource: metrics.SourceLocal, + wantRequestStatus: "success", + }, + { + name: "zero-length", + wantCacheStatus: XCacheHit, + wantContentLength: "0", + wantSource: metrics.SourceLocal, + wantRequestStatus: "success", + }, + } + for _, tt := range proxyCases { + t.Run(tt.name, func(t *testing.T) { + svc, _ := newWholeObjectCacheService(t, tt.objectBody, tt.cacheBody) + var upstreamCalls atomic.Int32 + svc.forwarder = &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{ + "Content-Length": []string{strconv.Itoa(len(tt.objectBody))}, + "Content-Type": []string{"text/plain"}, + "ETag": []string{`"etag"`}, + }, + Body: io.NopCloser(bytes.NewReader(tt.objectBody)), + ContentLength: int64(len(tt.objectBody)), + }, nil + }, + } + + labels := []struct { + status string + source string + }{ + {status: "success", source: metrics.SourceLocal}, + {status: "error", source: metrics.SourceLocal}, + {status: "success", source: metrics.SourceUpstream}, + {status: "error", source: metrics.SourceUpstream}, + } + before := make([]float64, len(labels)) + for i, label := range labels { + before[i] = requestCountForOperation(t, "GetObject", label.status, label.source) + } + + w := httptest.NewRecorder() + err := svc.HandleGetObject(w, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)) + if err != nil { + t.Errorf("body-length property %s: HandleGetObject returned %v, want nil", tt.name, err) + } + if w.Code != http.StatusOK { + t.Errorf("body-length property %s: status = %d, want %d", tt.name, w.Code, http.StatusOK) + } + if got := w.Header().Get(XCacheHeader); got != tt.wantCacheStatus { + t.Errorf("body-length property %s: %s = %q, want %q", tt.name, XCacheHeader, got, tt.wantCacheStatus) + } + if got := w.Header().Get("Content-Length"); got != tt.wantContentLength { + t.Errorf("body-length property %s: Content-Length = %q, want %q", tt.name, got, tt.wantContentLength) + } + if !bytes.Equal(w.Body.Bytes(), tt.wantBody) { + t.Errorf("body-length property %s: returned %d body bytes, want %d", tt.name, w.Body.Len(), len(tt.wantBody)) + } + if got := upstreamCalls.Load(); got != tt.wantUpstreamCalls { + t.Errorf("body-length property %s: upstream calls = %d, want %d", tt.name, got, tt.wantUpstreamCalls) + } + for i, label := range labels { + wantDelta := float64(0) + if label.source == tt.wantSource && label.status == tt.wantRequestStatus { + wantDelta = 1 + } + if got := requestCountForOperation(t, "GetObject", label.status, label.source); got != before[i]+wantDelta { + t.Errorf("body-length property %s: request count %s/%s delta = %v, want %v", tt.name, label.source, label.status, got-before[i], wantDelta) + } + } + }) + } + + tieredCases := []struct { + name string + objectBody []byte + cacheBody []byte + wantStatus int + wantCacheStatus string + wantContentLength string + wantBody []byte + wantNoSuchKey bool + }{ + { + name: "tiered-short-small", + objectBody: smallBody, + cacheBody: smallBody[:5], + wantStatus: http.StatusNotFound, + wantCacheStatus: XCacheMiss, + wantNoSuchKey: true, + }, + { + name: "tiered-short-large", + objectBody: largeBody, + cacheBody: largeBody[:32768], + wantStatus: http.StatusOK, + wantCacheStatus: XCacheHit, + wantContentLength: strconv.Itoa(len(largeBody)), + wantBody: largeBody[:32768], + }, + } + for _, tt := range tieredCases { + t.Run(tt.name, func(t *testing.T) { + svc, _, forwards, _ := newTieredShortBodyService(t, tt.objectBody, tt.cacheBody) + beforeErrors := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal) + beforeSuccesses := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal) + beforeForwards := forwards.Load() + w := httptest.NewRecorder() + err := svc.HandleGetObject(w, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)) + if err != nil { + t.Errorf("body-length property %s: HandleGetObject returned %v, want nil", tt.name, err) + } + if w.Code != tt.wantStatus { + t.Errorf("body-length property %s: status = %d, want %d", tt.name, w.Code, tt.wantStatus) + } + if got := w.Header().Get(XCacheHeader); got != tt.wantCacheStatus { + t.Errorf("body-length property %s: %s = %q, want %q", tt.name, XCacheHeader, got, tt.wantCacheStatus) + } + if got := w.Header().Get("Content-Length"); got != tt.wantContentLength { + t.Errorf("body-length property %s: Content-Length = %q, want %q", tt.name, got, tt.wantContentLength) + } + if tt.wantNoSuchKey { + if !bytes.Contains(w.Body.Bytes(), []byte("NoSuchKey")) { + t.Errorf("body-length property %s: response did not contain NoSuchKey: %q", tt.name, w.Body.String()) + } + if bytes.Contains(w.Body.Bytes(), tt.cacheBody) { + t.Errorf("body-length property %s: exposed short cache prefix %q", tt.name, tt.cacheBody) + } + } else if !bytes.Equal(w.Body.Bytes(), tt.wantBody) { + t.Errorf("body-length property %s: returned %d body bytes, want %d", tt.name, w.Body.Len(), len(tt.wantBody)) + } + if got := forwards.Load(); got != beforeForwards { + t.Errorf("body-length property %s: made %d upstream calls, want none", tt.name, got-beforeForwards) + } + if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("body-length property %s: local error count delta = %v, want 1", tt.name, got-beforeErrors) + } + if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("body-length property %s: local success count delta = %v, want 0", tt.name, got-beforeSuccesses) + } + }) + } + + t.Run("tiered-head-short", func(t *testing.T) { + objectBody := []byte("head body for local tier") + svc, _, forwards, _ := newTieredShortBodyService(t, objectBody, objectBody[:5]) + beforeErrors := requestCountForOperation(t, "HeadObject", "error", metrics.SourceLocal) + beforeSuccesses := requestCountForOperation(t, "HeadObject", "success", metrics.SourceLocal) + beforeForwards := forwards.Load() + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodHead, "/bucket/key", nil) + err := svc.HandleHeadObject(w, r) + if err != nil { + t.Errorf("body-length property tiered-head-short: HandleHeadObject returned %v, want nil", err) + } + if w.Code != http.StatusNotFound || w.Header().Get(XCacheHeader) != XCacheMiss { + t.Errorf("body-length property tiered-head-short: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) + } + if got := requestCountForOperation(t, "HeadObject", "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("body-length property tiered-head-short: local error count delta = %v, want 1", got-beforeErrors) + } + if got := requestCountForOperation(t, "HeadObject", "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("body-length property tiered-head-short: local success count delta = %v, want 0", got-beforeSuccesses) + } + if got := forwards.Load(); got != beforeForwards { + t.Errorf("body-length property tiered-head-short: made %d upstream calls, want none", got-beforeForwards) + } + }) + + t.Run("tiered-conditional-short", func(t *testing.T) { + objectBody := []byte("conditional body for local tier") + svc, _, forwards, meta := newTieredShortBodyService(t, objectBody, objectBody[:5]) + beforeErrors := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal) + beforeSuccesses := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal) + beforeForwards := forwards.Load() + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + r.Header.Set("If-None-Match", meta.ETag) + err := svc.HandleGetObject(w, r) + if err != nil { + t.Errorf("body-length property tiered-conditional-short: HandleGetObject returned %v, want nil", err) + } + if w.Code != http.StatusNotFound || w.Header().Get(XCacheHeader) != XCacheMiss { + t.Errorf("body-length property tiered-conditional-short: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) + } + if !bytes.Contains(w.Body.Bytes(), []byte("NoSuchKey")) { + t.Errorf("body-length property tiered-conditional-short: response did not contain NoSuchKey: %q", w.Body.String()) + } + if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("body-length property tiered-conditional-short: local error count delta = %v, want 1", got-beforeErrors) + } + if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("body-length property tiered-conditional-short: local success count delta = %v, want 0", got-beforeSuccesses) + } + if got := forwards.Load(); got != beforeForwards { + t.Errorf("body-length property tiered-conditional-short: made %d upstream calls, want none", got-beforeForwards) + } + }) + + t.Run("tiered-head-exact", func(t *testing.T) { + objectBody := []byte("exact head body") + svc, _, _, _ := newTieredShortBodyService(t, objectBody, objectBody) + w := httptest.NewRecorder() + err := svc.HandleHeadObject(w, httptest.NewRequest(http.MethodHead, "/bucket/key", nil)) + if err != nil || w.Code != http.StatusOK || w.Header().Get("Content-Length") != strconv.Itoa(len(objectBody)) { + t.Errorf("body-length property tiered-head-exact: response = %d/%q err=%v, want 200 and exact Content-Length", w.Code, w.Header().Get("Content-Length"), err) + } + }) + + t.Run("tiered-conditional-exact", func(t *testing.T) { + objectBody := []byte("exact conditional body") + svc, _, _, meta := newTieredShortBodyService(t, objectBody, objectBody) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + r.Header.Set("If-None-Match", meta.ETag) + err := svc.HandleGetObject(w, r) + if err != nil || w.Code != http.StatusNotModified { + t.Errorf("body-length property tiered-conditional-exact: response = %d err=%v, want 304", w.Code, err) + } + }) + + t.Run("tiered-ifmatch-short", func(t *testing.T) { + objectBody := []byte("if-match body") + svc, _, _, _ := newTieredShortBodyService(t, objectBody, objectBody[:5]) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + r.Header.Set("If-Match", `"not-the-etag"`) + err := svc.HandleGetObject(w, r) + if err != nil || w.Code != http.StatusPreconditionFailed { + t.Errorf("body-length property tiered-ifmatch-short: response = %d err=%v, want 412", w.Code, err) + } + if !bytes.Contains(w.Body.Bytes(), []byte("PreconditionFailed")) { + t.Errorf("body-length property tiered-ifmatch-short: response did not preserve PreconditionFailed: %q", w.Body.String()) + } + }) +} From 648fbe8776694b4a8949c9f1e6fed4c06fe02b99 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 09:13:00 +0000 Subject: [PATCH 02/13] test(proxy): cover rejected overlong cache streams Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 92 +++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index 16d65d7d..400203b8 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -9,6 +9,7 @@ import ( "strconv" "sync/atomic" "testing" + "time" dto "github.com/prometheus/client_model/go" cacheclient "github.com/tigrisdata/ocache/client" @@ -356,4 +357,95 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { t.Errorf("body-length property tiered-ifmatch-short: response did not preserve PreconditionFailed: %q", w.Body.String()) } }) + + for _, tc := range []struct { + name string + splitWrite bool + wantBody []byte + wantReadErr bool + }{ + {name: "overlong-whole-chunk", wantReadErr: true}, + {name: "overlong-second-chunk", splitWrite: true, wantBody: largeBody}, + } { + t.Run(tc.name, func(t *testing.T) { + extra := []byte(" extra") + overlong := append(append([]byte(nil), largeBody...), extra...) + stream := func(_ context.Context, _ string, w io.Writer) error { + if tc.splitWrite { + if _, err := w.Write(largeBody); err != nil { + return err + } + _, err := w.Write(extra) + return err + } + _, err := w.Write(overlong) + return err + } + cfg := config.NewDefault() + cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SetLegacyCoordination(false) + base := cacheclient.NewMemoryCache() + client := &testStreamCacheClient{CacheClient: base, stream: stream} + store := cache.NewCacheWithClient(client, &cfg.Cache) + meta := &cache.CachedObjectMeta{ + Bucket: "bucket", + Key: "key", + ETag: `"etag"`, + ContentType: "text/plain", + ContentLength: int64(len(largeBody)), + StatusCode: http.StatusOK, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, largeBody, 0); err != nil { + t.Fatalf("seed CAS cache: %v", err) + } + svc := NewService(&mockForwarder{}, store, cfg) + var upstreamCalls atomic.Int32 + svc.forwarder = &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return nil, errors.New("unexpected upstream request after commitment") + }, + } + beforeErrors := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal) + beforeSuccesses := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal) + handlerErr := make(chan error, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handlerErr <- svc.HandleGetObject(w, r) + })) + defer server.Close() + + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Get(server.URL + "/bucket/key") + if err != nil { + t.Fatalf("HTTP GET: %v", err) + } + responseBody, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if err := <-handlerErr; err != nil { + t.Errorf("body-length property %s: HandleGetObject returned %v after commitment", tc.name, err) + } + if resp.StatusCode != http.StatusOK || resp.Header.Get("Content-Length") != strconv.Itoa(len(largeBody)) { + t.Errorf("body-length property %s: response = %d Content-Length %q, want 200/%d", tc.name, resp.StatusCode, resp.Header.Get("Content-Length"), len(largeBody)) + } + if tc.wantReadErr { + if !errors.Is(readErr, io.ErrUnexpectedEOF) || len(responseBody) != 0 { + t.Errorf("body-length property %s: received %d bytes with read error %v, want zero bytes and unexpected EOF", tc.name, len(responseBody), readErr) + } + } else if readErr != nil || !bytes.Equal(responseBody, tc.wantBody) { + t.Errorf("body-length property %s: received %d bytes with read error %v, want the advertised body", tc.name, len(responseBody), readErr) + } + if got := upstreamCalls.Load(); got != 0 { + t.Errorf("body-length property %s: upstream calls = %d, want none after commitment", tc.name, got) + } + if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("body-length property %s: local error count delta = %v, want 1", tc.name, got-beforeErrors) + } + if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("body-length property %s: local success count delta = %v, want 0", tc.name, got-beforeSuccesses) + } + if _, found, err := store.GetMeta(context.Background(), meta.Bucket, meta.Key); err != nil || found { + t.Errorf("body-length property %s: CAS cleanup left metadata found=%v err=%v", tc.name, found, err) + } + }) + } } From c7519c332a48cf853d231e48f31d43e4cef87083 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 09:14:02 +0000 Subject: [PATCH 03/13] test(proxy): fix overlong writer test Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index 400203b8..57c545ae 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -3,6 +3,7 @@ package proxy import ( "bytes" "context" + "errors" "io" "net/http" "net/http/httptest" @@ -385,8 +386,8 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { cfg.Cache.SetBlockCachingEnabled(false) cfg.Cache.SetLegacyCoordination(false) base := cacheclient.NewMemoryCache() - client := &testStreamCacheClient{CacheClient: base, stream: stream} - store := cache.NewCacheWithClient(client, &cfg.Cache) + cacheClient := &testStreamCacheClient{CacheClient: base, stream: stream} + store := cache.NewCacheWithClient(cacheClient, &cfg.Cache) meta := &cache.CachedObjectMeta{ Bucket: "bucket", Key: "key", From fcb154c0f5a7c01682b23ba61726e3248a3885c0 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 09:30:09 +0000 Subject: [PATCH 04/13] test(proxy): use descriptive cache length assertions Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 80 ++++++++++++++++----------------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index 57c545ae..6ef91412 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -167,22 +167,22 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { w := httptest.NewRecorder() err := svc.HandleGetObject(w, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)) if err != nil { - t.Errorf("body-length property %s: HandleGetObject returned %v, want nil", tt.name, err) + t.Errorf("whole-object GET %s: HandleGetObject returned %v, want nil", tt.name, err) } if w.Code != http.StatusOK { - t.Errorf("body-length property %s: status = %d, want %d", tt.name, w.Code, http.StatusOK) + t.Errorf("whole-object GET %s: status = %d, want %d", tt.name, w.Code, http.StatusOK) } if got := w.Header().Get(XCacheHeader); got != tt.wantCacheStatus { - t.Errorf("body-length property %s: %s = %q, want %q", tt.name, XCacheHeader, got, tt.wantCacheStatus) + t.Errorf("whole-object GET %s: %s = %q, want %q", tt.name, XCacheHeader, got, tt.wantCacheStatus) } if got := w.Header().Get("Content-Length"); got != tt.wantContentLength { - t.Errorf("body-length property %s: Content-Length = %q, want %q", tt.name, got, tt.wantContentLength) + t.Errorf("whole-object GET %s: Content-Length = %q, want %q", tt.name, got, tt.wantContentLength) } if !bytes.Equal(w.Body.Bytes(), tt.wantBody) { - t.Errorf("body-length property %s: returned %d body bytes, want %d", tt.name, w.Body.Len(), len(tt.wantBody)) + t.Errorf("whole-object GET %s: returned %d body bytes, want %d", tt.name, w.Body.Len(), len(tt.wantBody)) } if got := upstreamCalls.Load(); got != tt.wantUpstreamCalls { - t.Errorf("body-length property %s: upstream calls = %d, want %d", tt.name, got, tt.wantUpstreamCalls) + t.Errorf("whole-object GET %s: upstream calls = %d, want %d", tt.name, got, tt.wantUpstreamCalls) } for i, label := range labels { wantDelta := float64(0) @@ -190,7 +190,7 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { wantDelta = 1 } if got := requestCountForOperation(t, "GetObject", label.status, label.source); got != before[i]+wantDelta { - t.Errorf("body-length property %s: request count %s/%s delta = %v, want %v", tt.name, label.source, label.status, got-before[i], wantDelta) + t.Errorf("whole-object GET %s: request count %s/%s delta = %v, want %v", tt.name, label.source, label.status, got-before[i], wantDelta) } } }) @@ -233,35 +233,35 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { w := httptest.NewRecorder() err := svc.HandleGetObject(w, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)) if err != nil { - t.Errorf("body-length property %s: HandleGetObject returned %v, want nil", tt.name, err) + t.Errorf("tiered whole-object GET %s: HandleGetObject returned %v, want nil", tt.name, err) } if w.Code != tt.wantStatus { - t.Errorf("body-length property %s: status = %d, want %d", tt.name, w.Code, tt.wantStatus) + t.Errorf("tiered whole-object GET %s: status = %d, want %d", tt.name, w.Code, tt.wantStatus) } if got := w.Header().Get(XCacheHeader); got != tt.wantCacheStatus { - t.Errorf("body-length property %s: %s = %q, want %q", tt.name, XCacheHeader, got, tt.wantCacheStatus) + t.Errorf("tiered whole-object GET %s: %s = %q, want %q", tt.name, XCacheHeader, got, tt.wantCacheStatus) } if got := w.Header().Get("Content-Length"); got != tt.wantContentLength { - t.Errorf("body-length property %s: Content-Length = %q, want %q", tt.name, got, tt.wantContentLength) + t.Errorf("tiered whole-object GET %s: Content-Length = %q, want %q", tt.name, got, tt.wantContentLength) } if tt.wantNoSuchKey { if !bytes.Contains(w.Body.Bytes(), []byte("NoSuchKey")) { - t.Errorf("body-length property %s: response did not contain NoSuchKey: %q", tt.name, w.Body.String()) + t.Errorf("tiered whole-object GET %s: response did not contain NoSuchKey: %q", tt.name, w.Body.String()) } if bytes.Contains(w.Body.Bytes(), tt.cacheBody) { - t.Errorf("body-length property %s: exposed short cache prefix %q", tt.name, tt.cacheBody) + t.Errorf("tiered whole-object GET %s: exposed short cache prefix %q", tt.name, tt.cacheBody) } } else if !bytes.Equal(w.Body.Bytes(), tt.wantBody) { - t.Errorf("body-length property %s: returned %d body bytes, want %d", tt.name, w.Body.Len(), len(tt.wantBody)) + t.Errorf("tiered whole-object GET %s: returned %d body bytes, want %d", tt.name, w.Body.Len(), len(tt.wantBody)) } if got := forwards.Load(); got != beforeForwards { - t.Errorf("body-length property %s: made %d upstream calls, want none", tt.name, got-beforeForwards) + t.Errorf("tiered whole-object GET %s: made %d upstream calls, want none", tt.name, got-beforeForwards) } if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { - t.Errorf("body-length property %s: local error count delta = %v, want 1", tt.name, got-beforeErrors) + t.Errorf("tiered whole-object GET %s: local error count delta = %v, want 1", tt.name, got-beforeErrors) } if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { - t.Errorf("body-length property %s: local success count delta = %v, want 0", tt.name, got-beforeSuccesses) + t.Errorf("tiered whole-object GET %s: local success count delta = %v, want 0", tt.name, got-beforeSuccesses) } }) } @@ -276,19 +276,19 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { r := httptest.NewRequest(http.MethodHead, "/bucket/key", nil) err := svc.HandleHeadObject(w, r) if err != nil { - t.Errorf("body-length property tiered-head-short: HandleHeadObject returned %v, want nil", err) + t.Errorf("tiered HEAD with a short body: HandleHeadObject returned %v, want nil", err) } if w.Code != http.StatusNotFound || w.Header().Get(XCacheHeader) != XCacheMiss { - t.Errorf("body-length property tiered-head-short: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) + t.Errorf("tiered HEAD with a short body: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) } if got := requestCountForOperation(t, "HeadObject", "error", metrics.SourceLocal); got != beforeErrors+1 { - t.Errorf("body-length property tiered-head-short: local error count delta = %v, want 1", got-beforeErrors) + t.Errorf("tiered HEAD with a short body: local error count delta = %v, want 1", got-beforeErrors) } if got := requestCountForOperation(t, "HeadObject", "success", metrics.SourceLocal); got != beforeSuccesses { - t.Errorf("body-length property tiered-head-short: local success count delta = %v, want 0", got-beforeSuccesses) + t.Errorf("tiered HEAD with a short body: local success count delta = %v, want 0", got-beforeSuccesses) } if got := forwards.Load(); got != beforeForwards { - t.Errorf("body-length property tiered-head-short: made %d upstream calls, want none", got-beforeForwards) + t.Errorf("tiered HEAD with a short body: made %d upstream calls, want none", got-beforeForwards) } }) @@ -303,22 +303,22 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { r.Header.Set("If-None-Match", meta.ETag) err := svc.HandleGetObject(w, r) if err != nil { - t.Errorf("body-length property tiered-conditional-short: HandleGetObject returned %v, want nil", err) + t.Errorf("tiered conditional GET with a short body: HandleGetObject returned %v, want nil", err) } if w.Code != http.StatusNotFound || w.Header().Get(XCacheHeader) != XCacheMiss { - t.Errorf("body-length property tiered-conditional-short: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) + t.Errorf("tiered conditional GET with a short body: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) } if !bytes.Contains(w.Body.Bytes(), []byte("NoSuchKey")) { - t.Errorf("body-length property tiered-conditional-short: response did not contain NoSuchKey: %q", w.Body.String()) + t.Errorf("tiered conditional GET with a short body: response did not contain NoSuchKey: %q", w.Body.String()) } if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { - t.Errorf("body-length property tiered-conditional-short: local error count delta = %v, want 1", got-beforeErrors) + t.Errorf("tiered conditional GET with a short body: local error count delta = %v, want 1", got-beforeErrors) } if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { - t.Errorf("body-length property tiered-conditional-short: local success count delta = %v, want 0", got-beforeSuccesses) + t.Errorf("tiered conditional GET with a short body: local success count delta = %v, want 0", got-beforeSuccesses) } if got := forwards.Load(); got != beforeForwards { - t.Errorf("body-length property tiered-conditional-short: made %d upstream calls, want none", got-beforeForwards) + t.Errorf("tiered conditional GET with a short body: made %d upstream calls, want none", got-beforeForwards) } }) @@ -328,7 +328,7 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { w := httptest.NewRecorder() err := svc.HandleHeadObject(w, httptest.NewRequest(http.MethodHead, "/bucket/key", nil)) if err != nil || w.Code != http.StatusOK || w.Header().Get("Content-Length") != strconv.Itoa(len(objectBody)) { - t.Errorf("body-length property tiered-head-exact: response = %d/%q err=%v, want 200 and exact Content-Length", w.Code, w.Header().Get("Content-Length"), err) + t.Errorf("tiered HEAD with an exact body: response = %d/%q err=%v, want 200 and exact Content-Length", w.Code, w.Header().Get("Content-Length"), err) } }) @@ -340,7 +340,7 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { r.Header.Set("If-None-Match", meta.ETag) err := svc.HandleGetObject(w, r) if err != nil || w.Code != http.StatusNotModified { - t.Errorf("body-length property tiered-conditional-exact: response = %d err=%v, want 304", w.Code, err) + t.Errorf("tiered conditional GET with an exact body: response = %d err=%v, want 304", w.Code, err) } }) @@ -352,10 +352,10 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { r.Header.Set("If-Match", `"not-the-etag"`) err := svc.HandleGetObject(w, r) if err != nil || w.Code != http.StatusPreconditionFailed { - t.Errorf("body-length property tiered-ifmatch-short: response = %d err=%v, want 412", w.Code, err) + t.Errorf("tiered GET with a failing If-Match: response = %d err=%v, want 412", w.Code, err) } if !bytes.Contains(w.Body.Bytes(), []byte("PreconditionFailed")) { - t.Errorf("body-length property tiered-ifmatch-short: response did not preserve PreconditionFailed: %q", w.Body.String()) + t.Errorf("tiered GET with a failing If-Match: response did not preserve PreconditionFailed: %q", w.Body.String()) } }) @@ -423,29 +423,29 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { responseBody, readErr := io.ReadAll(resp.Body) _ = resp.Body.Close() if err := <-handlerErr; err != nil { - t.Errorf("body-length property %s: HandleGetObject returned %v after commitment", tc.name, err) + t.Errorf("overlong whole-object GET %s: HandleGetObject returned %v after commitment", tc.name, err) } if resp.StatusCode != http.StatusOK || resp.Header.Get("Content-Length") != strconv.Itoa(len(largeBody)) { - t.Errorf("body-length property %s: response = %d Content-Length %q, want 200/%d", tc.name, resp.StatusCode, resp.Header.Get("Content-Length"), len(largeBody)) + t.Errorf("overlong whole-object GET %s: response = %d Content-Length %q, want 200/%d", tc.name, resp.StatusCode, resp.Header.Get("Content-Length"), len(largeBody)) } if tc.wantReadErr { if !errors.Is(readErr, io.ErrUnexpectedEOF) || len(responseBody) != 0 { - t.Errorf("body-length property %s: received %d bytes with read error %v, want zero bytes and unexpected EOF", tc.name, len(responseBody), readErr) + t.Errorf("overlong whole-object GET %s: received %d bytes with read error %v, want zero bytes and unexpected EOF", tc.name, len(responseBody), readErr) } } else if readErr != nil || !bytes.Equal(responseBody, tc.wantBody) { - t.Errorf("body-length property %s: received %d bytes with read error %v, want the advertised body", tc.name, len(responseBody), readErr) + t.Errorf("overlong whole-object GET %s: received %d bytes with read error %v, want the advertised body", tc.name, len(responseBody), readErr) } if got := upstreamCalls.Load(); got != 0 { - t.Errorf("body-length property %s: upstream calls = %d, want none after commitment", tc.name, got) + t.Errorf("overlong whole-object GET %s: upstream calls = %d, want none after commitment", tc.name, got) } if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { - t.Errorf("body-length property %s: local error count delta = %v, want 1", tc.name, got-beforeErrors) + t.Errorf("overlong whole-object GET %s: local error count delta = %v, want 1", tc.name, got-beforeErrors) } if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { - t.Errorf("body-length property %s: local success count delta = %v, want 0", tc.name, got-beforeSuccesses) + t.Errorf("overlong whole-object GET %s: local success count delta = %v, want 0", tc.name, got-beforeSuccesses) } if _, found, err := store.GetMeta(context.Background(), meta.Bucket, meta.Key); err != nil || found { - t.Errorf("body-length property %s: CAS cleanup left metadata found=%v err=%v", tc.name, found, err) + t.Errorf("overlong whole-object GET %s: CAS cleanup left metadata found=%v err=%v", tc.name, found, err) } }) } From 10da39f9f51f3fef8cef88dea86d346bbc4ce646 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 09:37:08 +0000 Subject: [PATCH 05/13] test(proxy): cover ranged conditional cache hits Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index 6ef91412..12858847 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -322,6 +322,37 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { } }) + t.Run("tiered-range-conditional-short", func(t *testing.T) { + objectBody := []byte("conditional range body for local tier") + svc, _, forwards, meta := newTieredShortBodyService(t, objectBody, objectBody[:5]) + beforeErrors := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal) + beforeSuccesses := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal) + beforeForwards := forwards.Load() + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + r.Header.Set("Range", "bytes=0-4") + r.Header.Set("If-None-Match", meta.ETag) + err := svc.HandleGetObject(w, r) + if err != nil { + t.Errorf("tiered ranged conditional GET with a short body: HandleGetObject returned %v, want nil", err) + } + if w.Code != http.StatusNotFound || w.Header().Get(XCacheHeader) != XCacheMiss { + t.Errorf("tiered ranged conditional GET with a short body: response = %d/%q, want 404/MISS", w.Code, w.Header().Get(XCacheHeader)) + } + if !bytes.Contains(w.Body.Bytes(), []byte("NoSuchKey")) || bytes.Contains(w.Body.Bytes(), objectBody[:5]) { + t.Errorf("tiered ranged conditional GET with a short body: response did not reject the short entry: %q", w.Body.String()) + } + if got := requestCountForOperation(t, "GetObject", "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("tiered ranged conditional GET with a short body: local error count delta = %v, want 1", got-beforeErrors) + } + if got := requestCountForOperation(t, "GetObject", "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("tiered ranged conditional GET with a short body: local success count delta = %v, want 0", got-beforeSuccesses) + } + if got := forwards.Load(); got != beforeForwards { + t.Errorf("tiered ranged conditional GET with a short body: made %d upstream calls, want none", got-beforeForwards) + } + }) + t.Run("tiered-head-exact", func(t *testing.T) { objectBody := []byte("exact head body") svc, _, _, _ := newTieredShortBodyService(t, objectBody, objectBody) @@ -344,6 +375,19 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { } }) + t.Run("tiered-range-conditional-exact", func(t *testing.T) { + objectBody := []byte("exact conditional range body") + svc, _, _, meta := newTieredShortBodyService(t, objectBody, objectBody) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + r.Header.Set("Range", "bytes=0-4") + r.Header.Set("If-None-Match", meta.ETag) + err := svc.HandleGetObject(w, r) + if err != nil || w.Code != http.StatusNotModified || w.Header().Get(XCacheHeader) != XCacheHit { + t.Errorf("tiered ranged conditional GET with an exact body: response = %d/%q err=%v, want 304/HIT", w.Code, w.Header().Get(XCacheHeader), err) + } + }) + t.Run("tiered-ifmatch-short", func(t *testing.T) { objectBody := []byte("if-match body") svc, _, _, _ := newTieredShortBodyService(t, objectBody, objectBody[:5]) From 40c2dd1a1aaff5e8e4b8a89a00c4ef88510b9d42 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 10:08:38 +0000 Subject: [PATCH 06/13] test(proxy): derive cache-length fixture sizes Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index 12858847..6967180c 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -19,6 +19,13 @@ import ( "github.com/tigrisdata/tag/metrics" ) +const shortCacheBodyPrefixSize = smallObjectThreshold / 2 + +func cacheTestBodyAboveSmallObjectThreshold(pattern string) []byte { + patternBytes := []byte(pattern) + return bytes.Repeat(patternBytes, smallObjectThreshold/len(patternBytes)+1) +} + func newWholeObjectCacheService(t *testing.T, objectBody, cacheBody []byte) (*Service, *cache.CachedObjectMeta) { t.Helper() @@ -69,7 +76,7 @@ func newTieredShortBodyService(t *testing.T, objectBody, cacheBody []byte) (*Ser func TestHandleGetObject_CacheBodyLength(t *testing.T) { smallBody := []byte("small cache body") - largeBody := bytes.Repeat([]byte("cached body"), 8192) + largeBody := cacheTestBodyAboveSmallObjectThreshold("cached body") proxyCases := []struct { name string objectBody []byte @@ -95,8 +102,8 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { { name: "short-large", objectBody: largeBody, - cacheBody: largeBody[:32768], - wantBody: largeBody[:32768], + cacheBody: largeBody[:shortCacheBodyPrefixSize], + wantBody: largeBody[:shortCacheBodyPrefixSize], wantCacheStatus: XCacheHit, wantContentLength: strconv.Itoa(len(largeBody)), wantSource: metrics.SourceLocal, @@ -217,11 +224,11 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { { name: "tiered-short-large", objectBody: largeBody, - cacheBody: largeBody[:32768], + cacheBody: largeBody[:shortCacheBodyPrefixSize], wantStatus: http.StatusOK, wantCacheStatus: XCacheHit, wantContentLength: strconv.Itoa(len(largeBody)), - wantBody: largeBody[:32768], + wantBody: largeBody[:shortCacheBodyPrefixSize], }, } for _, tt := range tieredCases { From b1ab9695c69a3255b5fe51e3422bdb1c7739fdf8 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 11:17:17 +0000 Subject: [PATCH 07/13] test(proxy): cover empty cache body mismatches Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index 6967180c..a5c04a97 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -99,6 +99,16 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { wantSource: metrics.SourceUpstream, wantRequestStatus: "success", }, + { + name: "short-empty-small", + objectBody: smallBody, + wantBody: smallBody, + wantCacheStatus: XCacheMiss, + wantContentLength: strconv.Itoa(len(smallBody)), + wantUpstreamCalls: 1, + wantSource: metrics.SourceUpstream, + wantRequestStatus: "success", + }, { name: "short-large", objectBody: largeBody, @@ -109,6 +119,16 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { wantSource: metrics.SourceLocal, wantRequestStatus: "error", }, + { + name: "short-empty-large", + objectBody: largeBody, + wantBody: largeBody, + wantCacheStatus: XCacheMiss, + wantContentLength: strconv.Itoa(len(largeBody)), + wantUpstreamCalls: 1, + wantSource: metrics.SourceUpstream, + wantRequestStatus: "success", + }, { name: "exact-small", objectBody: smallBody, From cd8f47c01ca5a4260c59772cceb3c3068b833be9 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 10:12:25 +0000 Subject: [PATCH 08/13] fix(proxy): reject incomplete whole-object cache hits Signed-off-by: Perfloop Agent --- README.md | 2 + cache/coordinator.go | 8 +- docs/architecture.md | 6 + docs/tiered-mode.md | 2 + metrics/metrics.go | 10 + proxy/block_cache.go | 20 + proxy/broadcast/broadcaster.go | 67 +++- proxy/broadcast/broadcaster_test.go | 141 +++++++ proxy/cache_hit_stream_test.go | 552 +++++++++++++++++++++++++++- proxy/caching.go | 25 ++ proxy/get_object.go | 79 ++-- proxy/localstore.go | 95 +++-- proxy/revalidation.go | 101 +++-- proxy/revalidation_cas_test.go | 16 +- proxy/revalidation_test.go | 33 +- proxy/tiered_test.go | 137 +++++++ 16 files changed, 1152 insertions(+), 142 deletions(-) diff --git a/README.md b/README.md index 4d85e42d..1ca4ac79 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,8 @@ For production, TAG ships manifests, Compose files, and guides under [`deploy/`] 2. **Cache Hit**: Returns cached object with `X-Cache: HIT` header 3. **Cache Miss**: Forwards request to upstream Tigris, caches response, returns with `X-Cache: MISS` +For positive-length whole-object cache hits, TAG checks the cache stream length against metadata. In transparent and signing modes, a short buffered GET falls back to upstream before headers commit. If that fallback fails before upstream headers arrive, TAG returns the error without committing the cached prefix. A committed short or overlong stream is counted as a local error and is not followed by an upstream body. Proxy HEAD and metadata-based conditional 304 responses retain their existing behavior. Tiered local GETs have no upstream body fallback: a buffered length mismatch returns the authoritative `NoSuchKey` miss, while a committed short or overlong stream is counted as an error and is not replaced. Positive-length tiered HEAD and conditional 304 responses verify the cached body length before answering from metadata. Version-guarded cleanup removes the observed row in CAS mode. The default legacy proxy coordinator refuses versioned deletion, so a damaged row may remain until replaced or expired; each subsequent GET still checks its length. + ### Request Coalescing When multiple concurrent requests arrive for the same uncached object: diff --git a/cache/coordinator.go b/cache/coordinator.go index 7c3baaac..d3a2bad0 100644 --- a/cache/coordinator.go +++ b/cache/coordinator.go @@ -369,10 +369,10 @@ func (c *legacyCoordinator) tombstoneTimestamp(ctx context.Context, bucket, key } // deleteMetaIfVersion is a CAS-strength identity delete: legacy coordination -// stores no versions (its tokens are wall-clock stamps never persisted with -// the row), so there is nothing to compare the caller's token against and the -// only safe answer is to refuse. Its sole caller (the tiered cleanup repair) -// runs under CAS coordination by construction — tiered mode rejects legacy. +// stores no versions (its tokens are wall-clock stamps never persisted with the +// row), so there is nothing to compare the caller's token against and the only +// safe answer is to refuse. Callers treat this as best-effort cleanup rather +// than risk deleting a same-ETag replacement through a non-atomic comparison. func (c *legacyCoordinator) deleteMetaIfVersion(ctx context.Context, bucket, key string, version uint64) (bool, error) { return false, nil } diff --git a/docs/architecture.md b/docs/architecture.md index 092de7b4..8d98ba60 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -206,6 +206,10 @@ Client TAG Embedded Cache │ X-Cache: HIT │ │ ``` +For a positive-length whole-object cache entry, TAG checks the cache stream length against metadata before counting a local GET as successful. In transparent and signing modes, a short buffered GET falls back upstream before response headers commit. If upstream fails before headers arrive, TAG returns the error without committing the cached prefix. A large stream may already be committed when a short or overlong mismatch is detected; TAG records a local error and does not append upstream bytes. Version-guarded cleanup removes the observed metadata row in CAS mode. The default legacy proxy coordinator refuses versioned deletion, so a damaged row may remain until replacement or expiry; later GETs still check the length. Proxy HEAD and metadata-based conditional 304 responses retain their existing metadata behavior. + +Tiered local reads have no upstream body fallback. A buffered length mismatch returns the authoritative `NoSuchKey` miss without cached bytes; a committed short or overlong stream is recorded as a local error without an upstream append. Positive-length tiered HEAD and conditional 304 responses also verify the exact body length with a final-byte range probe before returning metadata. Zero-length and unknown-length objects retain their existing paths. Range and block-cache reads use separate helpers. + ### GET Object (Cache Miss) ```text @@ -229,6 +233,8 @@ Client TAG Embedded Cache Tigris │ X-Cache: MISS │ │ │ ``` +If an upstream fetch fails before response headers are published, TAG returns the error before committing a success response. + ### GET Object (Cluster Mode - Remote Key) ```text diff --git a/docs/tiered-mode.md b/docs/tiered-mode.md index b2d2304c..f2e234a0 100644 --- a/docs/tiered-mode.md +++ b/docs/tiered-mode.md @@ -21,6 +21,8 @@ what makes the entry visible. `If-Match`/`If-None-Match` are honored; GET, HEAD, and DELETE are served entirely locally. Reads and writes of small objects generate zero upstream traffic. +For a positive-length whole-object local hit, TAG checks the cache stream length against metadata. A buffered length mismatch found before GET headers commit is discarded; TAG attempts version-guarded metadata cleanup, records a local request error, and returns the authoritative `NoSuchKey` miss so the caller can fall back to its system of record. If a large local body is found short or overlong after streaming commits headers, the response cannot be replaced: TAG records a local request error, attempts version-guarded metadata cleanup, and does not append an upstream body. Cleanup is best effort. Positive-length HEAD and conditional 304 responses also verify the exact body length with a final-byte range probe before answering from metadata; a confirmed mismatch returns the authoritative `NoSuchKey` miss instead of implying that the local body is servable. + **Large objects are the upstream tier.** The PUT passes through to upstream and TAG stores a metadata marker locally. HEAD answers from the marker; GET forwards for the body — the mode's only body traffic. DELETE forwards and diff --git a/metrics/metrics.go b/metrics/metrics.go index 0670551c..ee880342 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -482,6 +482,16 @@ func RecordRequest(operation, status, source string, durationSeconds float64) { RequestDuration.WithLabelValues(operation, processMode, source).Observe(durationSeconds) } +// RecordRequestOutcome maps an error to the shared request status label. Callers +// retain ownership of deciding whether their path produced an error. +func RecordRequestOutcome(operation, source string, durationSeconds float64, outcomeErr error) { + status := "success" + if outcomeErr != nil { + status = "error" + } + RecordRequest(operation, status, source, durationSeconds) +} + // RecordCacheHit records a cache hit. func RecordCacheHit() { CacheHits.Inc() diff --git a/proxy/block_cache.go b/proxy/block_cache.go index 0581d568..4664de5b 100644 --- a/proxy/block_cache.go +++ b/proxy/block_cache.go @@ -1403,6 +1403,26 @@ func (s *Service) invalidateStaleMeta(bucket, key, staleETag string) { } } +// invalidateMetaIfVersion removes only the metadata snapshot that the failed +// body read used. The coordinator owns the token semantics: CAS deletes exactly +// that version, while legacy coordination refuses a version delete rather than +// risk deleting a same-ETag replacement through its non-atomic ETag guard. +func (s *Service) invalidateMetaIfVersion(ctx context.Context, bucket, key string, version uint64) { + if !s.cache.IsEnabled() { + return + } + deleted, err := s.cache.DeleteMetaIfVersion(ctx, bucket, key, version) + if err != nil { + metrics.RecordCacheOperation("delete", "error") + log.Debug().Err(err).Str("bucket", bucket).Str("key", key). + Msg("Mismatched cache meta invalidation failed; entry may remain until TTL") + return + } + if deleted { + metrics.RecordCacheOperation("delete", "success") + } +} + // ensureBlocksCached makes covering blocks [b0,bK] present in cache: it probes the range once, // fetches any that are missing, and records per-block hits/misses plus whether the serve was a // full or partial hit. It returns an error only when a missing block could not be populated diff --git a/proxy/broadcast/broadcaster.go b/proxy/broadcast/broadcaster.go index c0374b4c..28dd50b5 100644 --- a/proxy/broadcast/broadcaster.go +++ b/proxy/broadcast/broadcaster.go @@ -62,6 +62,8 @@ const ( // The client will see a truncated response and S3 SDKs will automatically retry. var ErrSlowConsumer = errors.New("slow consumer") +var errCompletedBeforeHeaders = errors.New("broadcast completed before response headers") + // Chunk represents a piece of streamed data. // Consumers should call Release() after processing Data to return the // underlying buffer to the pool. If Release() is not called, the buffer @@ -90,24 +92,33 @@ type Listener struct { ch chan Chunk headers http.Header status int - headerCh chan struct{} // Closed when headers are available - disconnected bool // True if disconnected due to slow consumption + headerCh chan struct{} // Closed when headers or a pre-header completion error is available + headerErr error + disconnected bool // True if disconnected due to slow consumption } -// WaitForHeaders blocks until headers are available. If headers and context -// cancellation become ready together, headers win. A cache listener can be -// scheduled after its request handler returns, when both are already ready; it -// must still consume the completed response and publish the cache entry. +// WaitForHeaders blocks until headers are available or the broadcast completes +// before headers arrive. If headers and context cancellation become ready +// together, headers win. A cache listener can be scheduled after its request +// handler returns, when both are already ready; it must still consume the +// completed response and publish the cache entry. func (l *Listener) WaitForHeaders(ctx context.Context) (int, http.Header, error) { select { case <-l.headerCh: + if l.headerErr != nil { + return 0, nil, l.headerErr + } return l.status, l.headers, nil case <-ctx.Done(): // A select chooses randomly when both channels are ready. Re-check the - // headers so a completed response wins over concurrent cancellation, while - // still honoring cancellation when headers have not arrived. + // notification so a completed response or terminal error wins over + // concurrent cancellation, while still honoring cancellation when neither + // has arrived. select { case <-l.headerCh: + if l.headerErr != nil { + return 0, nil, l.headerErr + } return l.status, l.headers, nil default: return 0, nil, ctx.Err() @@ -167,7 +178,8 @@ func (b *Broadcaster) IsStreaming() bool { } // Subscribe adds a new listener to receive chunks. -// Returns nil if streaming has already started (no late joiners). +// Returns nil if streaming has already started (no late joiners). A completed +// broadcast that never streamed a body returns its terminal snapshot. func (b *Broadcaster) Subscribe() *Listener { b.mu.Lock() defer b.mu.Unlock() @@ -182,11 +194,24 @@ func (b *Broadcaster) Subscribe() *Listener { headerCh: make(chan struct{}), } - // If headers already set, copy them and close headerCh if b.headerSet { l.headers = b.headers.Clone() l.status = b.status close(l.headerCh) + } else if b.done { + l.headerErr = b.err + if l.headerErr == nil { + l.headerErr = errCompletedBeforeHeaders + } + close(l.headerCh) + } + + if b.done { + if b.headerSet && b.err != nil { + l.ch <- Chunk{Err: b.err} + } + close(l.ch) + return l } b.listeners = append(b.listeners, l) @@ -271,16 +296,34 @@ func (b *Broadcaster) Complete(err error) { b.done = true b.err = err + if !b.headerSet && b.err == nil { + b.err = errCompletedBeforeHeaders + } close(b.doneCh) - // Send final chunk and close all listener channels. + // A fetch failure before SetHeaders has no chunk for WaitForHeaders to + // observe. Publish the terminal error through the same readiness channel so + // every pre-header listener can return it without waiting for cancellation. + if !b.headerSet { + for _, l := range b.listeners { + l.headerErr = b.err + select { + case <-l.headerCh: + default: + close(l.headerCh) + } + } + } + + // Send final chunk and close all listener channels. After headers, errors + // remain in the chunk stream so listeners retain any already-queued prefix. // Non-blocking send: if buffer is full, skip the error chunk. // Correctness is guaranteed by the version preconditions on cache writes, // which prevents stale data from being cached even if error delivery fails. for _, l := range b.listeners { if !l.disconnected { select { - case l.ch <- Chunk{Err: err}: + case l.ch <- Chunk{Err: b.err}: default: } close(l.ch) diff --git a/proxy/broadcast/broadcaster_test.go b/proxy/broadcast/broadcaster_test.go index 8a63b844..c2e868d8 100644 --- a/proxy/broadcast/broadcaster_test.go +++ b/proxy/broadcast/broadcaster_test.go @@ -2,6 +2,7 @@ package broadcast import ( "context" + "errors" "net/http" "sync" "sync/atomic" @@ -280,6 +281,146 @@ func TestBroadcasterHeadersWinOverConcurrentCancellation(t *testing.T) { } } +func TestBroadcasterCompleteBeforeHeadersNotifiesListener(t *testing.T) { + b := NewBroadcaster(1) + listener := b.Subscribe() + wantErr := errors.New("upstream request failed") + + b.Complete(wantErr) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + assertPreHeaderError(t, listener, wantErr, ctx) +} + +func TestBroadcasterLateSubscriberReceivesPreHeaderError(t *testing.T) { + b := NewBroadcaster(1) + wantErr := errors.New("upstream request failed") + b.Complete(wantErr) + + listener := b.Subscribe() + if listener == nil { + t.Fatal("Subscribe returned nil for a completed non-streaming broadcast") + } + assertPreHeaderError(t, listener, wantErr, context.Background()) +} + +func TestBroadcasterLateSubscriberReceivesEmptyCompletion(t *testing.T) { + b := NewBroadcaster(1) + headers := http.Header{"Content-Length": []string{"0"}} + b.SetHeaders(http.StatusOK, headers) + b.Complete(nil) + + listener := b.Subscribe() + if listener == nil { + t.Fatal("Subscribe returned nil for a completed empty broadcast") + } + status, gotHeaders, err := listener.WaitForHeaders(context.Background()) + if err != nil { + t.Fatalf("WaitForHeaders: %v", err) + } + if status != http.StatusOK { + t.Errorf("status = %d, want %d", status, http.StatusOK) + } + if got := gotHeaders.Get("Content-Length"); got != "0" { + t.Errorf("Content-Length = %q, want 0", got) + } + select { + case chunk, ok := <-listener.Chunks(): + if ok && (chunk.Err != nil || len(chunk.Data) != 0) { + t.Fatalf("unexpected chunk after empty completion: %+v", chunk) + } + if ok { + select { + case _, ok := <-listener.Chunks(): + if ok { + t.Fatal("listener received more than one terminal chunk") + } + default: + t.Fatal("completed listener channel remains open") + } + } + default: + t.Fatal("completed listener channel remains open") + } +} + +func TestBroadcasterLateSubscriberReceivesPostHeaderError(t *testing.T) { + b := NewBroadcaster(1) + headers := http.Header{"Content-Length": []string{"3"}} + wantErr := errors.New("upstream body failed") + b.SetHeaders(http.StatusOK, headers) + b.Complete(wantErr) + + listener := b.Subscribe() + if listener == nil { + t.Fatal("Subscribe returned nil for a completed non-streaming broadcast") + } + status, gotHeaders, err := listener.WaitForHeaders(context.Background()) + if err != nil { + t.Fatalf("WaitForHeaders returned %v, want headers before body error", err) + } + if status != http.StatusOK || gotHeaders.Get("Content-Length") != "3" { + t.Fatalf("headers = %d/%v, want 200 and Content-Length 3", status, gotHeaders) + } + chunk, ok := <-listener.Chunks() + if !ok || !errors.Is(chunk.Err, wantErr) { + t.Fatalf("terminal chunk = %+v, open=%v, want %v", chunk, ok, wantErr) + } + if _, ok := <-listener.Chunks(); ok { + t.Fatal("listener channel remains open after terminal error") + } +} + +func TestBroadcasterCompleteAfterPrefixKeepsHeadersAndErrorInChunkOrder(t *testing.T) { + b := NewBroadcaster(2) + listener := b.Subscribe() + headers := http.Header{"Content-Length": []string{"6"}} + wantErr := errors.New("upstream body failed") + b.SetHeaders(http.StatusOK, headers) + b.Broadcast([]byte("foo")) + b.Complete(wantErr) + + status, gotHeaders, err := listener.WaitForHeaders(context.Background()) + if err != nil { + t.Fatalf("WaitForHeaders returned %v after headers were published", err) + } + if status != http.StatusOK || gotHeaders.Get("Content-Length") != "6" { + t.Fatalf("headers = %d/%v, want 200 and Content-Length 6", status, gotHeaders) + } + prefix, ok := <-listener.Chunks() + if !ok || string(prefix.Data) != "foo" || prefix.Err != nil { + t.Fatalf("first chunk = %+v, open=%v, want body prefix", prefix, ok) + } + prefix.Release() + terminal, ok := <-listener.Chunks() + if !ok || !errors.Is(terminal.Err, wantErr) { + t.Fatalf("terminal chunk = %+v, open=%v, want %v", terminal, ok, wantErr) + } + if _, ok := <-listener.Chunks(); ok { + t.Fatal("listener channel remains open after terminal error") + } +} + +func assertPreHeaderError(t *testing.T, listener *Listener, wantErr error, ctx context.Context) { + t.Helper() + + select { + case <-listener.headerCh: + default: + t.Fatal("pre-header completion did not notify the listener") + } + if _, _, err := listener.WaitForHeaders(ctx); !errors.Is(err, wantErr) { + t.Fatalf("WaitForHeaders error = %v, want %v", err, wantErr) + } + for chunk := range listener.Chunks() { + if chunk.Err != nil && !errors.Is(chunk.Err, wantErr) { + t.Errorf("terminal chunk error = %v, want %v", chunk.Err, wantErr) + } + chunk.Release() + } +} + func TestManagerGetOrCreate(t *testing.T) { m := NewManager(DefaultChannelBuffer) diff --git a/proxy/cache_hit_stream_test.go b/proxy/cache_hit_stream_test.go index 891f06b9..517123f8 100644 --- a/proxy/cache_hit_stream_test.go +++ b/proxy/cache_hit_stream_test.go @@ -7,6 +7,8 @@ import ( "io" "net/http" "net/http/httptest" + "strconv" + "sync" "sync/atomic" "testing" "time" @@ -20,18 +22,64 @@ import ( type testStreamCacheClient struct { cacheclient.CacheClient - stream func(context.Context, string, io.Writer) error + stream func(context.Context, string, io.Writer) error + rangeStream func(context.Context, string, int64, int64, io.Writer) error +} + +type originBodyReader struct { + io.ReadCloser + onError func(error) +} + +func (r *originBodyReader) Read(p []byte) (int, error) { + n, err := r.ReadCloser.Read(p) + if err != nil && !errors.Is(err, io.EOF) && r.onError != nil { + r.onError(err) + } + return n, err +} + +type commitTrackingResponseWriter struct { + *httptest.ResponseRecorder + committed bool +} + +func (w *commitTrackingResponseWriter) WriteHeader(statusCode int) { + w.committed = true + w.ResponseRecorder.WriteHeader(statusCode) +} + +func (w *commitTrackingResponseWriter) Write(p []byte) (int, error) { + w.committed = true + return w.ResponseRecorder.Write(p) +} + +func (w *commitTrackingResponseWriter) Flush() { + w.committed = true + w.ResponseRecorder.Flush() } func (c *testStreamCacheClient) GetStream(ctx context.Context, key string, w io.Writer) error { return c.stream(ctx, key, w) } +func (c *testStreamCacheClient) GetRangeStream(ctx context.Context, key string, start, end int64, w io.Writer) error { + if c.rangeStream != nil { + return c.rangeStream(ctx, key, start, end, w) + } + return c.CacheClient.GetRangeStream(ctx, key, start, end, w) +} + func newLargeStreamCacheService(t *testing.T, body []byte, stream func(context.Context, string, io.Writer) error) (*Service, *cache.Cache, *cache.CachedObjectMeta) { + return newLargeStreamCacheServiceWithCoordinator(t, body, stream, true) +} + +func newLargeStreamCacheServiceWithCoordinator(t *testing.T, body []byte, stream func(context.Context, string, io.Writer) error, legacy bool) (*Service, *cache.Cache, *cache.CachedObjectMeta) { t.Helper() cfg := config.NewDefault() cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SetLegacyCoordination(legacy) base := cacheclient.NewMemoryCache() client := &testStreamCacheClient{CacheClient: base, stream: stream} store := cache.NewCacheWithClient(client, &cfg.Cache) @@ -50,6 +98,83 @@ func newLargeStreamCacheService(t *testing.T, body []byte, stream func(context.C return NewService(&mockForwarder{}, store, cfg), store, meta } +func newWholeObjectCacheServiceWithCoordinator(t *testing.T, objectBody, cacheBody []byte, legacy bool) (*Service, *cache.CachedObjectMeta) { + t.Helper() + + cfg := config.NewDefault() + cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SetLegacyCoordination(legacy) + store := cache.NewCacheWithClient(cacheclient.NewMemoryCache(), &cfg.Cache) + meta := &cache.CachedObjectMeta{ + Bucket: "bucket", + Key: "key", + ETag: `"etag"`, + ContentType: "text/plain", + ContentLength: int64(len(objectBody)), + StatusCode: http.StatusOK, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, cacheBody, 0); err != nil { + t.Fatalf("seed whole-object cache: %v", err) + } + return NewService(&mockForwarder{}, store, cfg), meta +} + +func TestBodyLengthMatches(t *testing.T) { + ctx := context.Background() + cfg := config.NewDefault() + store := cache.NewCacheWithClient(cacheclient.NewMemoryCache(), &cfg.Cache) + svc := NewService(&mockForwarder{}, store, cfg) + tests := []struct { + name string + body []byte + length int64 + stored bool + wantMatch bool + }{ + {name: "one-byte-exact", body: []byte("a"), length: 1, stored: true, wantMatch: true}, + {name: "exact", body: []byte("abc"), length: 3, stored: true, wantMatch: true}, + {name: "short", body: []byte("ab"), length: 3, stored: true}, + {name: "empty", length: 3, stored: true}, + {name: "overlong", body: []byte("abcd"), length: 3, stored: true}, + {name: "absent", length: 3}, + } + + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + bucket, key, etag := "b", "key-"+strconv.Itoa(i), `"etag-`+strconv.Itoa(i)+`"` + meta := &cache.CachedObjectMeta{Bucket: bucket, Key: key, ETag: etag, ContentLength: tt.length} + if tt.stored { + if err := store.PutWithMeta(ctx, bucket, key, meta, tt.body, 60); err != nil { + t.Fatalf("PutWithMeta: %v", err) + } + } + got, err := svc.bodyLengthMatches(ctx, bucket, key, meta) + if err != nil || got != tt.wantMatch { + t.Fatalf("bodyLengthMatches() = (%v, %v), want (%v, nil)", got, err, tt.wantMatch) + } + }) + } +} + +func TestBodyLengthMatchesPropagatesTransientRangeError(t *testing.T) { + wantErr := errors.New("peer unavailable") + cfg := config.NewDefault() + client := &testStreamCacheClient{ + CacheClient: cacheclient.NewMemoryCache(), + rangeStream: func(context.Context, string, int64, int64, io.Writer) error { + return wantErr + }, + } + store := cache.NewCacheWithClient(client, &cfg.Cache) + svc := NewService(&mockForwarder{}, store, cfg) + meta := &cache.CachedObjectMeta{Bucket: "b", Key: "k", ETag: `"etag"`, ContentLength: 8} + + got, err := svc.bodyLengthMatches(context.Background(), meta.Bucket, meta.Key, meta) + if got || !errors.Is(err, wantErr) { + t.Fatalf("bodyLengthMatches() = (%v, %v), want (false, %v)", got, err, wantErr) + } +} + func TestServeFromCache_LargeObject(t *testing.T) { body := bytes.Repeat([]byte("cached body"), 8192) stream := func(_ context.Context, _ string, w io.Writer) error { @@ -66,7 +191,7 @@ func TestServeFromCache_LargeObject(t *testing.T) { svc, _, meta := newLargeStreamCacheService(t, body, stream) w := httptest.NewRecorder() - if err := svc.serveFromCache(context.Background(), w, meta.Bucket, meta.Key, meta, time.Now()); err != nil { + if err := svc.serveFromCache(context.Background(), w, meta.Bucket, meta.Key, meta, time.Now(), func() {}); err != nil { t.Fatalf("serveFromCache() error = %v", err) } if w.Code != http.StatusOK { @@ -89,7 +214,7 @@ func TestServeFromCache_LargeObjectUncommittedBeforeFirstByte(t *testing.T) { svc, _, meta := newLargeStreamCacheService(t, body, stream) w := httptest.NewRecorder() - err := svc.serveFromCache(context.Background(), w, meta.Bucket, meta.Key, meta, time.Now()) + err := svc.serveFromCache(context.Background(), w, meta.Bucket, meta.Key, meta, time.Now(), func() {}) if err == nil || !errors.Is(err, wantErr) { t.Fatalf("serveFromCache() error = %v, want wrapped %v", err, wantErr) } @@ -156,7 +281,7 @@ func TestHandleGetObject_LargeCacheHitPostCommitErrorDoesNotFallBack(t *testing. } return streamErr } - svc, _, _ := newLargeStreamCacheService(t, body, stream) + svc, _, _ := newLargeStreamCacheServiceWithCoordinator(t, body, stream, false) var upstreamCalls atomic.Int32 svc.forwarder = &mockForwarder{ @@ -189,15 +314,428 @@ func TestHandleGetObject_LargeCacheHitPostCommitErrorDoesNotFallBack(t *testing. if got := requestCount(t, "success"); got != beforeSuccesses { t.Fatalf("success request count = %v, want %v", got, beforeSuccesses) } + if _, found, err := svc.cache.GetMeta(context.Background(), "bucket", "key"); err != nil || !found { + t.Errorf("transient post-commit stream error removed valid metadata: found=%v err=%v", found, err) + } +} + +func TestServeFromCache_OverlongBodyIsNotSuccessfulInCAS(t *testing.T) { + smallObject := []byte("small object") + largeObject := cacheTestBodyAboveSmallObjectThreshold("cached body") + tests := []struct { + name string + objectBody []byte + cacheBody []byte + wantErr bool + wantBody []byte + wantCacheStatus string + wantContentLength int + wantRequestStatus string + }{ + { + name: "small", + objectBody: smallObject, + cacheBody: append(append([]byte(nil), smallObject...), []byte(" extra")...), + wantErr: true, + }, + { + name: "large", + objectBody: largeObject, + cacheBody: append(append([]byte(nil), largeObject...), []byte(" extra")...), + wantBody: append(append([]byte(nil), largeObject...), []byte(" extra")...), + wantCacheStatus: XCacheHit, + wantContentLength: len(largeObject), + wantRequestStatus: "error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, meta := newWholeObjectCacheServiceWithCoordinator(t, tt.objectBody, tt.cacheBody, false) + _, metaVersion, found, versionErr := svc.cache.GetMetaWithVersion(context.Background(), meta.Bucket, meta.Key) + if versionErr != nil || !found { + t.Fatalf("read seed metadata version: found=%v err=%v", found, versionErr) + } + beforeLocalSuccess := requestCountFrom(t, "success", metrics.SourceLocal) + beforeLocalErrors := requestCountFrom(t, "error", metrics.SourceLocal) + w := &commitTrackingResponseWriter{ResponseRecorder: httptest.NewRecorder()} + err := svc.serveFromCache(context.Background(), w, meta.Bucket, meta.Key, meta, time.Now(), func() { + svc.invalidateMetaIfVersion(context.Background(), meta.Bucket, meta.Key, metaVersion) + }) + if (err != nil) != tt.wantErr { + t.Fatalf("serveFromCache() error = %v, want error %v", err, tt.wantErr) + } + if _, found, err := svc.cache.GetMeta(context.Background(), meta.Bucket, meta.Key); err != nil || found { + t.Errorf("CAS cleanup left overlong metadata: found=%v err=%v", found, err) + } + if tt.wantErr { + if w.committed || w.Body.Len() != 0 || w.Header().Get(XCacheHeader) != "" { + t.Errorf("short-path response committed: headers=%v body=%q", w.Header(), w.Body.String()) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeLocalSuccess { + t.Errorf("local success count delta = %v, want 0", got-beforeLocalSuccess) + } + return + } + if !w.committed { + t.Fatal("large-path response was not committed") + } + if got := w.Header().Get(XCacheHeader); got != tt.wantCacheStatus { + t.Errorf("%s = %q, want %q", XCacheHeader, got, tt.wantCacheStatus) + } + if got := w.Header().Get("Content-Length"); got != strconv.Itoa(tt.wantContentLength) { + t.Errorf("Content-Length = %q, want %d", got, tt.wantContentLength) + } + if !bytes.Equal(w.Body.Bytes(), tt.wantBody) { + t.Errorf("body length = %d, want %d", w.Body.Len(), len(tt.wantBody)) + } + if got := requestCountFrom(t, tt.wantRequestStatus, metrics.SourceLocal); got != beforeLocalErrors+1 { + t.Errorf("local error count delta = %v, want 1", got-beforeLocalErrors) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeLocalSuccess { + t.Errorf("local success count delta = %v, want 0", got-beforeLocalSuccess) + } + }) + } +} + +func TestHandleGetObject_ShortBodyCleanupPreservesSameETagReplacement(t *testing.T) { + body := cacheTestBodyAboveSmallObjectThreshold("cached body") + prefix := body[:shortCacheBodyPrefixSize] + for _, tc := range []struct { + name string + legacy bool + }{ + {name: "legacy default", legacy: true}, + {name: "CAS coordinator", legacy: false}, + } { + t.Run(tc.name, func(t *testing.T) { + started := make(chan struct{}) + release := make(chan struct{}) + var releaseOnce sync.Once + releaseStream := func() { releaseOnce.Do(func() { close(release) }) } + defer releaseStream() + cfg := config.NewDefault() + cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SetLegacyCoordination(tc.legacy) + base := cacheclient.NewMemoryCache() + var streamOnce sync.Once + stream := func(ctx context.Context, key string, w io.Writer) error { + first := false + var streamErr error + streamOnce.Do(func() { + first = true + if _, err := w.Write(prefix); err != nil { + streamErr = err + close(started) + return + } + close(started) + select { + case <-release: + case <-ctx.Done(): + streamErr = ctx.Err() + } + }) + if first { + return streamErr + } + return base.GetStream(ctx, key, w) + } + client := &testStreamCacheClient{CacheClient: base, stream: stream} + store := cache.NewCacheWithClient(client, &cfg.Cache) + meta := &cache.CachedObjectMeta{ + Bucket: "bucket", + Key: "key", + ETag: `"etag"`, + ContentType: "text/plain", + ContentLength: int64(len(body)), + StatusCode: http.StatusOK, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, body, 0); err != nil { + t.Fatalf("seed cache: %v", err) + } + svc := NewService(&mockForwarder{}, store, cfg) + var upstreamCalls atomic.Int32 + svc.forwarder = &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return nil, errors.New("unexpected upstream request after commitment") + }, + } + beforeErrors := requestCountFrom(t, "error", metrics.SourceLocal) + beforeSuccesses := requestCountFrom(t, "success", metrics.SourceLocal) + w := httptest.NewRecorder() + done := make(chan error, 1) + go func() { + done <- svc.HandleGetObject(w, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)) + }() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("old cache stream did not reach its first committed chunk") + } + + replacement := &cache.CachedObjectMeta{ + Bucket: meta.Bucket, + Key: meta.Key, + ETag: meta.ETag, + ContentType: meta.ContentType, + ContentLength: meta.ContentLength, + StatusCode: meta.StatusCode, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, replacement, body, 0); err != nil { + t.Fatalf("commit same-ETag replacement: %v", err) + } + releaseStream() + if err := <-done; err != nil { + t.Fatalf("HandleGetObject() error = %v, want nil after committed stream", err) + } + + if w.Code != http.StatusOK || w.Header().Get(XCacheHeader) != XCacheHit || w.Header().Get("Content-Length") != strconv.Itoa(len(body)) { + t.Errorf("older response = %d headers=%v, want committed HIT with metadata length", w.Code, w.Header()) + } + if !bytes.Equal(w.Body.Bytes(), prefix) { + t.Errorf("older response body length = %d, want short prefix length %d", w.Body.Len(), len(prefix)) + } + if got := upstreamCalls.Load(); got != 0 { + t.Errorf("post-commit upstream calls = %d, want 0", got) + } + if got := requestCountFrom(t, "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("local error count delta = %v, want 1", got-beforeErrors) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("local success count delta = %v, want 0", got-beforeSuccesses) + } + + current, found, err := store.GetMeta(context.Background(), meta.Bucket, meta.Key) + if err != nil || !found || current == nil || current.ETag != meta.ETag { + t.Fatalf("same-ETag replacement metadata was removed: current=%+v found=%v err=%v", current, found, err) + } + var currentBody bytes.Buffer + if err := store.GetBodyStream(context.Background(), meta.Bucket, meta.Key, current.BodyDiscriminator(), ¤tBody); err != nil { + t.Fatalf("read replacement body: %v", err) + } + if !bytes.Equal(currentBody.Bytes(), body) { + t.Errorf("replacement body length = %d, want %d", currentBody.Len(), len(body)) + } + }) + } +} + +func TestHandleGetObject_LargeShortCacheBodyInvalidatesBeforeNextGetInCAS(t *testing.T) { + body := cacheTestBodyAboveSmallObjectThreshold("cached body") + prefix := body[:shortCacheBodyPrefixSize] + svc, meta := newWholeObjectCacheServiceWithCoordinator(t, body, prefix, false) + var upstreamCalls atomic.Int32 + svc.forwarder = &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{ + "Content-Length": []string{strconv.Itoa(len(body))}, + "Content-Type": []string{"text/plain"}, + "ETag": []string{`"refetched"`}, + }, + Body: io.NopCloser(bytes.NewReader(body)), + ContentLength: int64(len(body)), + }, nil + }, + } + + beforeLocalErrors := requestCountFrom(t, "error", metrics.SourceLocal) + beforeLocalSuccesses := requestCountFrom(t, "success", metrics.SourceLocal) + beforeUpstreamSuccesses := requestCountFrom(t, "success", metrics.SourceUpstream) + first := httptest.NewRecorder() + if err := svc.HandleGetObject(first, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)); err != nil { + t.Fatalf("first HandleGetObject() error = %v", err) + } + if first.Code != http.StatusOK || first.Header().Get(XCacheHeader) != XCacheHit { + t.Fatalf("first response = %d X-Cache %q, want 200 HIT", first.Code, first.Header().Get(XCacheHeader)) + } + if first.Header().Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(first.Body.Bytes(), prefix) { + t.Fatalf("first committed response length=%q body_bytes=%d, want length %d and prefix %d", first.Header().Get("Content-Length"), first.Body.Len(), len(body), len(prefix)) + } + if got := upstreamCalls.Load(); got != 0 { + t.Fatalf("first GET made %d upstream calls, want 0 after commitment", got) + } + if _, found, err := svc.cache.GetMeta(context.Background(), meta.Bucket, meta.Key); err != nil || found { + t.Fatalf("mismatched cache metadata remains after committed response: found=%v err=%v", found, err) + } + if got := requestCountFrom(t, "error", metrics.SourceLocal); got != beforeLocalErrors+1 { + t.Errorf("first local error count delta = %v, want 1", got-beforeLocalErrors) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeLocalSuccesses { + t.Errorf("first local success count delta = %v, want 0", got-beforeLocalSuccesses) + } + + second := httptest.NewRecorder() + if err := svc.HandleGetObject(second, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)); err != nil { + t.Fatalf("second HandleGetObject() error = %v", err) + } + if second.Code != http.StatusOK || second.Header().Get(XCacheHeader) != XCacheMiss { + t.Fatalf("second response = %d X-Cache %q, want 200 MISS", second.Code, second.Header().Get(XCacheHeader)) + } + if second.Header().Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(second.Body.Bytes(), body) { + t.Fatalf("second response length=%q body_bytes=%d, want %d complete bytes", second.Header().Get("Content-Length"), second.Body.Len(), len(body)) + } + if got := upstreamCalls.Load(); got != 1 { + t.Errorf("total upstream calls = %d, want 1", got) + } + if got := requestCountFrom(t, "success", metrics.SourceUpstream); got != beforeUpstreamSuccesses+1 { + t.Errorf("upstream success count delta = %v, want 1", got-beforeUpstreamSuccesses) + } +} + +func TestHandleGetObject_RevalidationLargeShortBodyInvalidatesMetadataInCAS(t *testing.T) { + body := cacheTestBodyAboveSmallObjectThreshold("cached body") + prefix := body[:shortCacheBodyPrefixSize] + tests := []struct { + name string + conditionalResp *http.Response + conditionalErr error + }{ + { + name: "not-modified", + conditionalResp: &http.Response{ + StatusCode: http.StatusNotModified, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(nil)), + }, + }, + { + name: "stale-after-error", + conditionalErr: errors.New("upstream revalidation failed"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, meta := newWholeObjectCacheServiceWithCoordinator(t, body, prefix, false) + var fallbackCalls atomic.Int32 + forwarder := &mockForwarder{ + conditionalResp: tt.conditionalResp, + conditionalErr: tt.conditionalErr, + forwardFunc: func(context.Context, http.ResponseWriter, *http.Request) error { + fallbackCalls.Add(1) + return errors.New("unexpected fallback after committed cache response") + }, + } + svc.forwarder = forwarder + beforeErrors := requestCountFrom(t, "error", metrics.SourceLocal) + beforeSuccesses := requestCountFrom(t, "success", metrics.SourceLocal) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + r.Header.Set("Cache-Control", "no-cache") + if err := svc.HandleGetObject(w, r); err != nil { + t.Fatalf("HandleGetObject() error = %v", err) + } + if !forwarder.conditionalCalled { + t.Fatal("HandleGetObject did not take the revalidation path") + } + if got := fallbackCalls.Load(); got != 0 { + t.Errorf("post-commit fallback calls = %d, want 0", got) + } + if w.Code != http.StatusOK || w.Header().Get(XCacheHeader) != XCacheHit || w.Header().Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(w.Body.Bytes(), prefix) { + t.Errorf("committed response = %d headers=%v body_bytes=%d, want HIT with short prefix and recorded length", w.Code, w.Header(), w.Body.Len()) + } + if _, found, err := svc.cache.GetMeta(context.Background(), meta.Bucket, meta.Key); err != nil || found { + t.Errorf("mismatched metadata remains after revalidation serve: found=%v err=%v", found, err) + } + if got := requestCountFrom(t, "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("local error count delta = %v, want 1", got-beforeErrors) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("local success count delta = %v, want 0", got-beforeSuccesses) + } + }) + } +} + +func TestHandleGetObject_ShortCacheBodyUpstreamFailureBeforeCommit(t *testing.T) { + objectBody := []byte("complete object body") + cacheBody := objectBody[:5] + svc, _ := newWholeObjectCacheService(t, objectBody, cacheBody) + wantErr := errors.New("upstream request failed") + var upstreamCalls atomic.Int32 + svc.forwarder = &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return nil, wantErr + }, + } + + beforeLocalSuccess := requestCountFrom(t, "success", metrics.SourceLocal) + beforeUpstreamErrors := requestCountFrom(t, "error", metrics.SourceUpstream) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + w := &commitTrackingResponseWriter{ResponseRecorder: httptest.NewRecorder()} + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil).WithContext(ctx) + err := svc.HandleGetObject(w, r) + + if !errors.Is(err, wantErr) { + t.Fatalf("HandleGetObject() error = %v, want %v", err, wantErr) + } + if got := upstreamCalls.Load(); got != 1 { + t.Errorf("upstream calls = %d, want 1", got) + } + if w.committed || w.Body.Len() != 0 || w.Header().Get(XCacheHeader) != "" { + t.Errorf("response committed before the upstream error: committed=%v code=%d headers=%v body=%q", w.committed, w.Code, w.Header(), w.Body.String()) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeLocalSuccess { + t.Errorf("local success count delta = %v, want 0", got-beforeLocalSuccess) + } + if got := requestCountFrom(t, "error", metrics.SourceUpstream); got != beforeUpstreamErrors+1 { + t.Errorf("upstream error count delta = %v, want 1", got-beforeUpstreamErrors) + } +} + +func TestHandleGetObject_CompletedPreHeaderBroadcastReturnsError(t *testing.T) { + wantErr := errors.New("upstream request failed") + var upstreamCalls atomic.Int32 + forwarder := &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return nil, wantErr + }, + } + svc, _ := newTestService(forwarder, true) + broadcastKey := makeBroadcastKey("bucket", "key", "") + broadcaster, first := svc.broadcastManager.GetOrCreate(broadcastKey) + if !first { + t.Fatal("initial GetOrCreate did not create a broadcaster") + } + broadcaster.Complete(wantErr) + defer svc.broadcastManager.Remove(broadcastKey) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + w := &commitTrackingResponseWriter{ResponseRecorder: httptest.NewRecorder()} + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil).WithContext(ctx) + err := svc.HandleGetObject(w, r) + + if !errors.Is(err, wantErr) { + t.Fatalf("HandleGetObject() error = %v, want %v", err, wantErr) + } + if got := upstreamCalls.Load(); got != 0 { + t.Errorf("late listener started %d upstream fetches, want 0", got) + } + if w.committed || w.Body.Len() != 0 || w.Header().Get(XCacheHeader) != "" { + t.Errorf("response committed before the broadcast error: committed=%v code=%d headers=%v body=%q", w.committed, w.Code, w.Header(), w.Body.String()) + } } func requestCount(t *testing.T, status string) float64 { + return requestCountFrom(t, status, metrics.SourceLocal) +} + +func requestCountFrom(t *testing.T, status, source string) float64 { t.Helper() var metric dto.Metric - // mode defaults to "unknown" in tests (SetMode is a main.go concern); the - // cache-hit paths this test exercises record source=local. - if err := metrics.RequestsTotal.WithLabelValues("GetObject", status, "unknown", metrics.SourceLocal).Write(&metric); err != nil { + // mode defaults to "unknown" in tests (SetMode is a main.go concern). + if err := metrics.RequestsTotal.WithLabelValues("GetObject", status, "unknown", source).Write(&metric); err != nil { t.Fatalf("read %s request count: %v", status, err) } return metric.GetCounter().GetValue() diff --git a/proxy/caching.go b/proxy/caching.go index cd82233f..9848eec9 100644 --- a/proxy/caching.go +++ b/proxy/caching.go @@ -102,6 +102,31 @@ func bodyGone(err error) bool { return true } +// bodyLengthMatches checks a positive, declared whole-body length without +// reading the entire body. The inclusive range [length-1,length] yields one +// byte for an exact body, no bytes for a short body, and two for an overlong +// body. The in-memory client reports an out-of-range start as InvalidArgument; +// the range is valid by construction here, so that response proves the body is +// shorter than its metadata. +func (s *Service) bodyLengthMatches(ctx context.Context, bucket, key string, meta *cache.CachedObjectMeta) (bool, error) { + if meta.ContentLength <= 0 { + return false, nil + } + + cw := &countingWriter{w: io.Discard} + err := s.cache.GetRangeStream(ctx, bucket, key, meta.BodyDiscriminator(), meta.ContentLength-1, meta.ContentLength, cw) + if err != nil { + if errors.Is(err, cache.ErrNotFound) { + return false, nil + } + if st, ok := status.FromError(err); ok && st.Code() == codes.InvalidArgument { + return false, nil + } + return false, err + } + return cw.written == 1, nil +} + // cacheWriteTimeoutForSize returns a timeout scaled to contentLength. // Returns at least cacheWriteTimeout (60s), scaling up for large objects. func cacheWriteTimeoutForSize(contentLength int64) time.Duration { diff --git a/proxy/get_object.go b/proxy/get_object.go index ed3c1731..45204b29 100644 --- a/proxy/get_object.go +++ b/proxy/get_object.go @@ -54,13 +54,15 @@ func (cw *countingWriter) Write(p []byte) (int, error) { return n, err } -// lazyCommitWriter counts a cache body stream and commits the response when -// the first nonempty chunk arrives. Empty and failed streams can therefore be -// handled by the cache-miss fallback without sending cache headers first. +// lazyCommitWriter counts bytes offered by the cache and commits the response +// when the first nonempty chunk arrives. Empty and failed streams can therefore +// be handled by the cache-miss fallback without sending cache headers first. type lazyCommitWriter struct { w http.ResponseWriter meta *cache.CachedObjectMeta - written int64 + written int64 // bytes accepted by the downstream writer + offered int64 // bytes read from the cache, including a chunk the writer rejects + overlong bool committed bool } @@ -68,6 +70,10 @@ func (cw *lazyCommitWriter) Write(p []byte) (int, error) { if len(p) == 0 { return 0, nil } + cw.offered += int64(len(p)) + if cw.meta.ContentLength > 0 && cw.offered > cw.meta.ContentLength { + cw.overlong = true + } if !cw.committed { cw.meta.WriteHeaders(cw.w) writeCacheStatus(cw.w, XCacheHit) @@ -77,6 +83,9 @@ func (cw *lazyCommitWriter) Write(p []byte) (int, error) { n, err := cw.w.Write(p) cw.written += int64(n) + if err == nil && n != len(p) { + return n, io.ErrShortWrite + } return n, err } @@ -122,7 +131,7 @@ func (s *Service) HandleGetObject(w http.ResponseWriter, r *http.Request) error isAnonymous := isAnonymousRequest(r, result, err) if (result == AuthValidated || isAnonymous) && !bypassCache && s.cache.IsEnabled() { - meta, found, cacheErr := s.cache.GetMeta(ctx, bucket, key) + meta, metaVersion, found, cacheErr := s.cache.GetMetaWithVersion(ctx, bucket, key) cacheHit := cacheErr == nil && found && meta != nil // Anonymous requests can only be served from cache if the object's ACL allows it if cacheHit && isAnonymous && !meta.IsPublicRead() { @@ -141,7 +150,7 @@ func (s *Service) HandleGetObject(w http.ResponseWriter, r *http.Request) error Str("bucket", bucket). Str("key", key). Msg("Cache hit requires revalidation (client-triggered)") - return s.revalidateAndServe(ctx, w, r, bucket, key, accessKey, secretKey, meta, start) + return s.revalidateAndServe(ctx, w, r, bucket, key, accessKey, secretKey, meta, metaVersion, start) } // If the client forced revalidation but there is no whole-body entry to revalidate @@ -219,15 +228,16 @@ func (s *Service) HandleGetObject(w http.ResponseWriter, r *http.Request) error return assembleErr } // Fall through to cache miss path - } else if cacheBodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start); cacheBodyErr != nil { + } else if cacheBodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start, func() { + s.invalidateMetaIfVersion(ctx, bucket, key, metaVersion) + }); cacheBodyErr != nil { log.Warn().Err(cacheBodyErr).Str("bucket", bucket).Str("key", key).Msg("Cache body unavailable, falling through to upstream") - // Invalidate only when the body is genuinely gone: the metadata is then - // orphaned, and clearing it stops a meta-hit/body-miss loop that repeats - // the failed cache read on every request before refetching. A transient - // failure (e.g. the client disconnected mid-read) must not evict a - // still-valid hot entry. serveFromCache errors before committing - // headers, so falling through to the miss path below is safe either way. - if bodyGone(cacheBodyErr) { + // Invalidate genuinely missing bodies to stop a meta-hit/body-miss loop. + // Length mismatches already used the served snapshot's version guard; + // do not retry them with ETag, which can match a replacement. Transient + // read failures also leave the entry alone. serveFromCache errors before + // committing, so the miss path below is still safe. + if bodyGone(cacheBodyErr) && !errors.Is(cacheBodyErr, errCacheBodyLengthMismatch) { s.invalidateStaleMeta(bucket, key, meta.ETag) } // Fall through to cache miss path @@ -864,27 +874,30 @@ func (s *Service) handleRangeWithBackgroundCache( return nil } +// cacheMetaMatchesNotModifiedRequest reports whether cached metadata satisfies +// the request's 304 validators without committing a response. Per RFC 7232 §3.3, +// If-None-Match is judged alone; If-Modified-Since is ignored when it is present. +// LastModified has only second precision, so falling back to the date could +// 304 across a same-second overwrite that the ETag would reject. +func cacheMetaMatchesNotModifiedRequest(r *http.Request, meta *cache.CachedObjectMeta) bool { + if inm := r.Header.Get("If-None-Match"); inm != "" { + return meta.MatchesETagHeader(inm, false) + } + ims := r.Header.Get("If-Modified-Since") + if ims == "" { + return false + } + t, parseErr := http.ParseTime(ims) + return parseErr == nil && !meta.IsModifiedSince(t) +} + // writeNotModifiedFromCache answers a conditional request with 304 when the -// cached entry satisfies the request's validators. Per RFC 7232 §3.3, a -// request carrying If-None-Match is judged by it ALONE — If-Modified-Since is -// ignored, matching or not: LastModified is second-granular, so falling back -// to it could 304 a client across a same-second overwrite it should see. -// Returns true when it wrote the response. Shared by the proxying GET hit -// path and the origin-less handler so the two cannot drift. +// cached entry satisfies the request's validators. Returns true when it wrote +// the response. Shared by the proxying GET hit path and the origin-less handler +// so the two cannot drift. func (s *Service) writeNotModifiedFromCache(w http.ResponseWriter, r *http.Request, meta *cache.CachedObjectMeta, operation string, start time.Time) bool { - if inm := r.Header.Get("If-None-Match"); inm != "" { - if !meta.MatchesETagHeader(inm, false) { - return false - } - } else { - ims := r.Header.Get("If-Modified-Since") - if ims == "" { - return false - } - t, parseErr := http.ParseTime(ims) - if parseErr != nil || meta.IsModifiedSince(t) { - return false - } + if !cacheMetaMatchesNotModifiedRequest(r, meta) { + return false } writeCacheStatus(w, XCacheHit) w.Header().Set("ETag", meta.ETag) diff --git a/proxy/localstore.go b/proxy/localstore.go index 762c8369..30f61c19 100644 --- a/proxy/localstore.go +++ b/proxy/localstore.go @@ -10,6 +10,7 @@ import ( "crypto/md5" "encoding/base64" "encoding/hex" + "errors" "fmt" "io" "net/http" @@ -106,17 +107,9 @@ func (s *Service) serveOriginlessObject(w http.ResponseWriter, r *http.Request, return s.originlessMiss(w, r, operation, start) } - // ONE existence gate for the whole handler: an entry is visible only when its - // data is fully present — every block of a block-mode entry, the body of a - // whole-object one. Every answer below (304, HEAD 200, any serve) implies - // existence, and each of HEAD, conditionals, and the serve paths independently - // answering that question is exactly how three review rounds of - // existence-vs-serveability disagreements happened. An incomplete entry is - // simply invisible: metadata alone cannot be served, and claiming existence - // from it makes HEAD-as-existence callers — anything that skips re-population - // when a HEAD says the object exists — skip the healing that would make the - // entry servable again. A probe-to-serve race can still truncate a - // concurrent eviction; the gate narrows the window, nothing can close it. + // First rule out a genuinely absent local body. Metadata-only answers then + // verify the declared end byte below; a full GET checks the bytes it actually + // reads in serveFromCache. servable, servErr := s.entryServable(ctx, bucket, key, meta) if servErr != nil { // A transient probe failure is not absence: 500-retry, never a false miss. @@ -127,10 +120,35 @@ func (s *Service) serveOriginlessObject(w http.ResponseWriter, r *http.Request, return s.originlessMiss(w, r, operation, start) } - // Conditional requests are answered from the cached metadata; there is no - // upstream for a client's Cache-Control to revalidate against, so no-cache and - // no-store are simply not consulted — the cached copy is the only copy. - if s.answerConditionalsFromMeta(w, r, meta, operation, start) { + // Preserve S3 precondition precedence before checking body length. A failed + // If-Match/If-Unmodified-Since answers 412 from metadata even when the body + // itself is short; it does not claim that the body can be served. + if writePreconditionFailed(w, r, meta) { + metrics.RecordRequest(operation, "success", metrics.SourceLocal, time.Since(start).Seconds()) + return nil + } + + // HEAD and conditional GETs whose validators match may be answered from + // metadata alone, including a Range request that returns 304 before reading + // its range. For a positive declared length, verify the final byte before + // returning HEAD 200 or a cached 304; body GETs are checked while serving. + metadataOnly := r.Method == http.MethodHead || + (r.Method == http.MethodGet && cacheMetaMatchesNotModifiedRequest(r, meta)) + if metadataOnly && meta.ContentLength > 0 { + lengthMatches, probeErr := s.bodyLengthMatches(ctx, bucket, key, meta) + if probeErr != nil { + metrics.RecordRequestOutcome(operation, metrics.SourceLocal, time.Since(start).Seconds(), probeErr) + return probeErr + } + if !lengthMatches { + s.invalidateMetaIfVersion(ctx, bucket, key, metaVersion) + return s.originlessMissWithRequestError(w, r, operation, start, errCacheBodyLengthMismatch) + } + } + + // Conditional requests are answered from cached metadata; there is no + // upstream for this mode to revalidate against. + if s.writeNotModifiedFromCache(w, r, meta, operation, start) { return nil } @@ -151,29 +169,31 @@ func (s *Service) serveOriginlessObject(w http.ResponseWriter, r *http.Request, return s.finishServeBodyError(ctx, w, r, bucket, key, metaVersion, rangeErr, operation, start) } - if bodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start); bodyErr != nil { - // serveFromCache fails before committing headers, so a miss response is + if bodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start, func() { + // The stream has committed, so guarded cleanup must not turn the partial + // response into a miss response. + s.invalidateMetaIfVersion(ctx, bucket, key, metaVersion) + }); bodyErr != nil { + // serveFromCache errors before committing headers, so a miss response is // still writable. return s.finishServeBodyError(ctx, w, r, bucket, key, metaVersion, bodyErr, operation, start) } return nil } -// finishServeBodyError disposes of a pre-commit body-serve failure. It -// distinguishes two cases the mode must not conflate: -// - The body is GENUINELY GONE (evicted under a live meta): an -// authoritative miss, and the orphaned meta is invalidated — but -// ETag-GUARDED (DeleteIfETag), never an unconditional delete, so a -// concurrent PUT's just-committed newer version is never wiped. The -// guard resolves against the served snapshot's discriminator identity; -// a different current entry keeps the key. -// - Anything else is TRANSIENT (a cluster-peer gRPC blip, a deadline): -// NOT absence. Minting an authoritative NoSuchKey here would tell the -// caller a live object does not exist — so it propagates as a retryable -// 5xx, matching this handler's meta-read and probe legs. +// finishServeBodyError disposes of a pre-commit body-serve failure. A confirmed +// length mismatch is an authoritative miss counted as an error; its invalidation +// uses the version read with the served snapshot. A genuinely missing body is an +// authoritative miss with best-effort version cleanup. Other failures (for +// example, a peer error or deadline) are transient and remain retryable errors. func (s *Service) finishServeBodyError(ctx context.Context, w http.ResponseWriter, r *http.Request, bucket, key string, metaVersion uint64, serveErr error, operation string, start time.Time) error { + if errors.Is(serveErr, errCacheBodyLengthMismatch) { + // serveFromCache already attempted version-guarded cleanup before + // returning this pre-commit mismatch. + return s.originlessMissWithRequestError(w, r, operation, start, serveErr) + } if !bodyGone(serveErr) { - metrics.RecordRequest(operation, "error", metrics.SourceLocal, time.Since(start).Seconds()) + metrics.RecordRequestOutcome(operation, metrics.SourceLocal, time.Since(start).Seconds(), serveErr) return serveErr } // VERSION-guarded against the SERVED snapshot: a concurrent PUT's newer @@ -188,10 +208,9 @@ func (s *Service) finishServeBodyError(ctx context.Context, w http.ResponseWrite return s.originlessMiss(w, r, operation, start) } -// entryServable is the handler's single existence answer: all blocks present for -// a block-mode entry, the body present for a whole-object one. Everything the -// handler says — 304, HEAD 200, a served body — flows from this one predicate, so -// existence and serveability cannot disagree. +// entryServable is the inexpensive first-byte presence check for a whole-body +// local entry. Positive-length HEAD and conditional answers additionally verify +// the declared end byte; body GETs verify the full count while serving. func (s *Service) entryServable(ctx context.Context, bucket, key string, meta *cache.CachedObjectMeta) (bool, error) { // A zero-length object is vacuously servable: no byte can be missing, and the // first-byte probe below cannot see one anyway — the embedded backend returns @@ -206,9 +225,15 @@ func (s *Service) entryServable(ctx context.Context, bucket, key string, meta *c // originlessMiss answers the one thing a miss can be in this mode. func (s *Service) originlessMiss(w http.ResponseWriter, r *http.Request, operation string, start time.Time) error { + return s.originlessMissWithRequestError(w, r, operation, start, nil) +} + +// originlessMissWithRequestError emits the authoritative NoSuchKey response and +// records the outcome that led to the miss through the shared metrics mapping. +func (s *Service) originlessMissWithRequestError(w http.ResponseWriter, r *http.Request, operation string, start time.Time, requestErr error) error { writeCacheStatus(w, XCacheMiss) s3err.WriteError(w, r, s3err.ErrNoSuchKey) - metrics.RecordRequest(operation, "success", metrics.SourceLocal, time.Since(start).Seconds()) + metrics.RecordRequestOutcome(operation, metrics.SourceLocal, time.Since(start).Seconds(), requestErr) return nil } diff --git a/proxy/revalidation.go b/proxy/revalidation.go index 245baed6..b1fbb20a 100644 --- a/proxy/revalidation.go +++ b/proxy/revalidation.go @@ -3,6 +3,7 @@ package proxy import ( "bytes" "context" + "errors" "fmt" "io" "net/http" @@ -14,6 +15,8 @@ import ( "github.com/tigrisdata/tag/metrics" ) +var errCacheBodyLengthMismatch = errors.New("cache body length mismatch") + // revalidateAndServe sends a conditional GET to upstream and serves the result. // Supports both full-object and range requests. For range requests, the Range header // is included in the conditional GET so upstream returns 304 or 206. @@ -26,6 +29,7 @@ func (s *Service) revalidateAndServe( r *http.Request, bucket, key, accessKey, secretKey string, meta *cache.CachedObjectMeta, + metaVersion uint64, start time.Time, ) error { metrics.RecordRevalidationTriggered() @@ -47,7 +51,7 @@ func (s *Service) revalidateAndServe( // Upstream error — serve stale from cache log.Warn().Err(err).Str("bucket", bucket).Str("key", key).Msg("Revalidation failed, serving stale") metrics.RecordRevalidationFailed() - served, staleErr := s.serveStaleFromCache(ctx, w, r, bucket, key, meta, rangeHeader, start) + served, staleErr := s.serveStaleFromCache(ctx, w, r, bucket, key, meta, metaVersion, rangeHeader, start) if served { metrics.RecordRevalidationStaleServed() return staleErr @@ -60,7 +64,7 @@ func (s *Service) revalidateAndServe( switch resp.StatusCode { case http.StatusNotModified: - served, revalErr := s.handleRevalidation304(ctx, w, r, bucket, key, meta, rangeHeader, start) + served, revalErr := s.handleRevalidation304(ctx, w, r, bucket, key, meta, metaVersion, rangeHeader, start) if served { // A response (full body, range, or a committed-then-failed stream) was // produced; propagate its result. Headers are already sent on error, so @@ -88,7 +92,7 @@ func (s *Service) revalidateAndServe( Str("key", key). Msg("Revalidation got unexpected status, serving stale") metrics.RecordRevalidationFailed() - served, staleErr := s.serveStaleFromCache(ctx, w, r, bucket, key, meta, rangeHeader, start) + served, staleErr := s.serveStaleFromCache(ctx, w, r, bucket, key, meta, metaVersion, rangeHeader, start) if served { metrics.RecordRevalidationStaleServed() return staleErr @@ -112,8 +116,10 @@ func (s *Service) forwardAfterCacheMiss(ctx context.Context, w http.ResponseWrit // because the normal re-warm paths are gated on the metadata being absent. // Deleting it makes the next request a clean miss that repopulates the cache. // A transient failure (e.g. a canceled context from a disconnected client) must - // not evict a still-valid entry, so bodyGone gates the delete. - if s.cache.IsEnabled() && bodyGone(bodyErr) { + // not evict a still-valid entry, so bodyGone gates the delete. A length + // mismatch already used the served snapshot's version guard; it must not be + // retried here through the weaker ETag comparison. + if s.cache.IsEnabled() && bodyGone(bodyErr) && !errors.Is(bodyErr, errCacheBodyLengthMismatch) { // ETag-guarded: only the entry whose body was observed gone is removed, // never one a concurrent request re-established under a newer version. s.invalidateStaleMeta(bucket, key, staleETag) @@ -141,19 +147,22 @@ func (s *Service) handleRevalidation304( r *http.Request, bucket, key string, meta *cache.CachedObjectMeta, + metaVersion uint64, rangeHeader string, start time.Time, ) (served bool, err error) { metrics.RecordRevalidationNotModified() log.Debug().Str("bucket", bucket).Str("key", key).Msg("Revalidation 304 - object unchanged") - // Serve range or full body from cache. Both serveRangeFromCache (via its - // pre-header probe) and serveFromCache report an unresolvable body without - // writing headers, so the caller can safely forward to upstream. + // Serve range or full body from cache. The range helper reports served=true + // once it commits, and the whole-object helper returns nil after a committed + // stream; only a pre-commit failure leaves the caller free to fetch upstream. if rangeHeader != "" { return s.serveRangeFromCache(ctx, w, r, bucket, key, meta, rangeHeader, start) } - if bodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start); bodyErr != nil { + if bodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start, func() { + s.invalidateMetaIfVersion(ctx, bucket, key, metaVersion) + }); bodyErr != nil { return false, bodyErr } return true, nil @@ -311,14 +320,16 @@ func (s *Service) handleRevalidation200( return copyErr } -// serveFromCache serves an object from the cache body. -// Used as fallback during revalidation (304 or error paths). +// serveFromCache serves a whole object from the cache body. Buffered length +// mismatches return before headers commit; a mismatch found after a streamed +// response commits is accounted locally and never triggers an upstream append. func (s *Service) serveFromCache( ctx context.Context, w http.ResponseWriter, bucket, key string, meta *cache.CachedObjectMeta, start time.Time, + invalidateOnLengthMismatch func(), ) error { // Zero-byte objects: no body to serve if meta.ContentLength == 0 { @@ -326,13 +337,21 @@ func (s *Service) serveFromCache( return nil } - // Small objects: buffer and serve + // Small objects: buffer and verify before committing the response. if meta.ContentLength <= smallObjectThreshold { bodyBuf := bufferPool.Get().(*bytes.Buffer) bodyBuf.Reset() bodyErr := s.cache.GetBodyStream(ctx, bucket, key, meta.BodyDiscriminator(), bodyBuf) if bodyErr == nil && bodyBuf.Len() > 0 { + if meta.ContentLength > 0 && int64(bodyBuf.Len()) != meta.ContentLength { + bodyLen := bodyBuf.Len() + putBuffer(bodyBuf) + if invalidateOnLengthMismatch != nil { + invalidateOnLengthMismatch() + } + return fmt.Errorf("%w for %s/%s: got %d bytes, want %d", errCacheBodyLengthMismatch, bucket, key, bodyLen, meta.ContentLength) + } meta.WriteHeaders(w) writeCacheStatus(w, XCacheHit) w.WriteHeader(meta.StatusCode) @@ -350,31 +369,45 @@ func (s *Service) serveFromCache( return fmt.Errorf("cache body empty for %s/%s", bucket, key) } - // Large objects: stream directly to a writer that commits only when the - // cache produces its first nonempty chunk. This preserves the pre-commit - // fallback for an absent or empty body without staging the stream through a - // pipe and a second copy. + // Large objects stream directly to a writer that commits on its first + // nonempty chunk. The writer tracks bytes offered by the cache separately + // from bytes accepted downstream, so a Content-Length rejection still proves + // an overlong cache entry. cw := &lazyCommitWriter{w: w, meta: meta} bodyErr := s.cache.GetBodyStream(ctx, bucket, key, meta.BodyDiscriminator(), cw) - status := "success" - if bodyErr != nil { - if !cw.committed { - return fmt.Errorf("cache body unavailable: %w", bodyErr) + if bodyErr != nil && !cw.committed { + return fmt.Errorf("cache body unavailable: %w", bodyErr) + } + if !cw.committed { + return fmt.Errorf("cache body empty for %s/%s", bucket, key) + } + + var requestErr error + lengthMismatch := cw.overlong || (bodyErr == nil && meta.ContentLength > 0 && cw.written != meta.ContentLength) + if lengthMismatch { + log.Warn().Err(bodyErr). + Str("bucket", bucket). + Str("key", key). + Int64("expected_bytes", meta.ContentLength). + Int64("written_bytes", cw.written). + Int64("offered_bytes", cw.offered). + Msg("Cache body length mismatch after headers committed") + if invalidateOnLengthMismatch != nil { + invalidateOnLengthMismatch() } + requestErr = errCacheBodyLengthMismatch + } else if bodyErr != nil { // The response is already committed. Do not return the error to the - // caller: HandleGetObject would otherwise try to append an upstream - // response to the partial cached body. The response is incomplete, - // however, so record the request as an error. + // caller: HandleGetObject would otherwise append an upstream response to + // the partial cache body. A non-length stream or writer error is not + // proof that the cache entry itself is damaged. log.Warn().Err(bodyErr).Str("bucket", bucket).Str("key", key). Msg("Failed to stream cache body after headers committed") - status = "error" - } - if !cw.committed { - return fmt.Errorf("cache body empty for %s/%s", bucket, key) + requestErr = bodyErr } metrics.BytesTransferred.WithLabelValues("out").Add(float64(cw.written)) - metrics.RecordRequest("GetObject", status, metrics.SourceLocal, time.Since(start).Seconds()) + metrics.RecordRequestOutcome("GetObject", metrics.SourceLocal, time.Since(start).Seconds(), requestErr) return nil } @@ -433,22 +466,26 @@ func (s *Service) handleRevalidation206Range( } // serveStaleFromCache serves stale content from cache, handling both full and -// range requests. It returns served=false, without writing any response bytes, -// when the cached body cannot be resolved so the caller can fall back to a direct -// upstream fetch instead of emitting a truncated or empty response. +// range requests. It returns served=false only when no response was committed, +// allowing the caller to fetch upstream. A committed stream that later proves +// incomplete is accounted for by the serve helper and cannot be replaced by an +// upstream response. func (s *Service) serveStaleFromCache( ctx context.Context, w http.ResponseWriter, r *http.Request, bucket, key string, meta *cache.CachedObjectMeta, + metaVersion uint64, rangeHeader string, start time.Time, ) (served bool, err error) { if rangeHeader != "" { return s.serveRangeFromCache(ctx, w, r, bucket, key, meta, rangeHeader, start) } - if bodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start); bodyErr != nil { + if bodyErr := s.serveFromCache(ctx, w, bucket, key, meta, start, func() { + s.invalidateMetaIfVersion(ctx, bucket, key, metaVersion) + }); bodyErr != nil { return false, bodyErr } return true, nil diff --git a/proxy/revalidation_cas_test.go b/proxy/revalidation_cas_test.go index f5449bfa..d378276a 100644 --- a/proxy/revalidation_cas_test.go +++ b/proxy/revalidation_cas_test.go @@ -72,7 +72,7 @@ func TestRevalidation200_ReplacesStaleEntryAfterFailedDelete(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/b/k", nil) - if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, time.Now()); err != nil { + if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, revalidationMetaVersion(t, svc, stale), time.Now()); err != nil { t.Fatalf("revalidateAndServe: %v", err) } if w.Body.String() != newBody { @@ -135,7 +135,7 @@ func TestRevalidation206_BackgroundFetchReplacesStaleAfterFailedDelete(t *testin r.Header.Set("Cache-Control", "no-cache") // A credentialed request, so the background repair uses the signed fetch. r.Header.Set("Authorization", "AWS4-HMAC-SHA256 Credential=access/20260907/auto/s3/aws4_request") - if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, time.Now()); err != nil { + if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, revalidationMetaVersion(t, svc, stale), time.Now()); err != nil { t.Fatalf("revalidateAndServe: %v", err) } if w.Code != http.StatusPartialContent { @@ -326,15 +326,16 @@ func TestRevalidation200_TokenFailureSkipsCaching(t *testing.T) { if err := c.PutWithMeta(ctx, bucket, key, stale, []byte("stale"), 0); err != nil { t.Fatalf("seed: %v", err) } + metaVersion := revalidationMetaVersion(t, svc, stale) // Fail exactly the picker's read. GetWithVersion call ledger: the seed's - // PutWithMeta VersionAny loop is #1, the guarded delete's own read is #2 - // (succeeds — the stale row is removed and fenced), the picker's is #3. - wrapped.failOn = 3 + // PutWithMeta VersionAny loop is #1, the request snapshot is #2, the guarded + // delete's own read is #3, and the picker's read is #4. + wrapped.failOn = 4 w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/b/k", nil) - if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, time.Now()); err != nil { + if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, metaVersion, time.Now()); err != nil { t.Fatalf("revalidateAndServe: %v", err) } if w.Body.String() != newBody { @@ -379,6 +380,7 @@ func TestRevalidation200_FreshRacerSkipsCaching(t *testing.T) { if err := c.PutWithMeta(ctx, bucket, key, stale, []byte("stale"), 0); err != nil { t.Fatalf("seed: %v", err) } + metaVersion := revalidationMetaVersion(t, svc, stale) // After the guarded delete clears the stale row, a racer re-establishes // the entry with a different ETag, and is then fenced-deleted — the exact @@ -401,7 +403,7 @@ func TestRevalidation200_FreshRacerSkipsCaching(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/b/k", nil) - if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, time.Now()); err != nil { + if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", stale, metaVersion, time.Now()); err != nil { t.Fatalf("revalidateAndServe: %v", err) } if w.Body.String() != newBody { diff --git a/proxy/revalidation_test.go b/proxy/revalidation_test.go index be50dfeb..d89b0a5c 100644 --- a/proxy/revalidation_test.go +++ b/proxy/revalidation_test.go @@ -125,6 +125,15 @@ func newTestService(forwarder RequestForwarder, cacheEnabled bool) (*Service, *c return svc, c } +func revalidationMetaVersion(t *testing.T, svc *Service, meta *cache.CachedObjectMeta) uint64 { + t.Helper() + _, version, _, err := svc.cache.GetMetaWithVersion(context.Background(), meta.Bucket, meta.Key) + if err != nil { + t.Fatalf("GetMetaWithVersion: %v", err) + } + return version +} + func TestRevalidation304_ServesCachedBody(t *testing.T) { // Setup: mock upstream returns 304 Not Modified mock := &mockForwarder{ @@ -157,7 +166,7 @@ func TestRevalidation304_ServesCachedBody(t *testing.T) { // Execute revalidation w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) - err = svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err = svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -212,7 +221,7 @@ func TestRevalidation200_StreamsNewBody(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -262,7 +271,7 @@ func TestRevalidation200_UncacheableDeletesStale(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -304,7 +313,7 @@ func TestRevalidationError_ServesStale(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -347,7 +356,7 @@ func TestRevalidationUnexpectedStatus_ServesStale(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -499,7 +508,7 @@ func TestServeFromCache_ZeroByteObject(t *testing.T) { } w := httptest.NewRecorder() - err := svc.serveFromCache(context.Background(), w, "b", "k", meta, time.Now()) + err := svc.serveFromCache(context.Background(), w, "b", "k", meta, time.Now(), func() {}) if err != nil { t.Fatalf("serveFromCache() error = %v", err) } @@ -533,7 +542,7 @@ func TestServeFromCache_SmallObject(t *testing.T) { _ = c.PutWithMeta(ctx, bucket, key, meta, body, 0) w := httptest.NewRecorder() - err := svc.serveFromCache(ctx, w, bucket, key, meta, time.Now()) + err := svc.serveFromCache(ctx, w, bucket, key, meta, time.Now(), func() {}) if err != nil { t.Fatalf("serveFromCache() error = %v", err) } @@ -579,7 +588,7 @@ func TestRevalidationRange304_ServesCachedRange(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) r.Header.Set("Range", "bytes=0-4") - err = svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err = svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -651,7 +660,7 @@ func TestRevalidationRange304_BodyEvictedForwardsUpstream(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) r.Header.Set("Range", "bytes=0-4") - if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()); err != nil { + if err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()); err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -709,7 +718,7 @@ func TestRevalidationRange206_StreamsRangeFromUpstream(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) r.Header.Set("Range", "bytes=0-9") - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -756,7 +765,7 @@ func TestRevalidationRangeError_ServesStaleRange(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) r.Header.Set("Range", "bytes=0-4") - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v", err) } @@ -817,7 +826,7 @@ func TestRevalidation304_CacheBodyUnavailable_FallsThrough(t *testing.T) { // Execute revalidation — should fall through to Forward w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/test-bucket/test-key", nil) - err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, time.Now()) + err := svc.revalidateAndServe(ctx, w, r, bucket, key, "access", "secret", meta, revalidationMetaVersion(t, svc, meta), time.Now()) if err != nil { t.Fatalf("revalidateAndServe() error = %v (expected fallback to upstream)", err) } diff --git a/proxy/tiered_test.go b/proxy/tiered_test.go index c3d5e8ae..f4f05638 100644 --- a/proxy/tiered_test.go +++ b/proxy/tiered_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/http/httptest" + "strconv" "strings" "sync" "sync/atomic" @@ -19,6 +20,7 @@ import ( cacheclient "github.com/tigrisdata/ocache/client" "github.com/tigrisdata/tag/cache" "github.com/tigrisdata/tag/config" + "github.com/tigrisdata/tag/metrics" ) // newTieredTestService builds a Service in tiered mode with an in-memory cache @@ -1081,6 +1083,141 @@ func TestTieredStreamedPutBodyRefRoundTrip(t *testing.T) { } } +func TestTieredLargeShortBodyInvalidatesForNextGet(t *testing.T) { + body := string(cacheTestBodyAboveSmallObjectThreshold("cached body")) + prefix := body[:shortCacheBodyPrefixSize] + mock, forwards, _ := tieredMock() + svc, store := newTieredTestService(mock, int64(len(body)+1)) + if w := tieredDo(t, svc, http.MethodPut, "/bucket/key", body, nil); w.Code != http.StatusOK { + t.Fatalf("local PUT status = %d", w.Code) + } + meta, found, err := store.GetMeta(context.Background(), "bucket", "key") + if err != nil || !found || meta == nil || meta.BodyRef == "" { + t.Fatalf("local metadata = %+v found=%v err=%v", meta, found, err) + } + if err := store.PutBodyStream(context.Background(), "bucket", "key", meta.BodyRef, strings.NewReader(prefix), 0); err != nil { + t.Fatalf("truncate local-tier body: %v", err) + } + + beforeErrors := requestCountFrom(t, "error", metrics.SourceLocal) + first := tieredDo(t, svc, http.MethodGet, "/bucket/key", "", nil) + if first.Code != http.StatusOK || first.Header().Get(XCacheHeader) != XCacheHit || first.Header().Get("Content-Length") != strconv.Itoa(len(body)) || first.Body.String() != string(prefix) { + t.Fatalf("first GET = %d headers=%v body_bytes=%d, want HIT with short prefix", first.Code, first.Header(), first.Body.Len()) + } + if _, found, err := store.GetMeta(context.Background(), "bucket", "key"); err != nil || found { + t.Fatalf("short local-tier metadata remains: found=%v err=%v", found, err) + } + if got := requestCountFrom(t, "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("local error count delta = %v, want 1", got-beforeErrors) + } + if got := forwards.Load(); got != 0 { + t.Errorf("tiered upstream forwards = %d, want 0", got) + } + + second := tieredDo(t, svc, http.MethodGet, "/bucket/key", "", nil) + if second.Code != http.StatusNotFound || second.Header().Get(XCacheHeader) != XCacheMiss { + t.Fatalf("second GET = %d X-Cache=%q, want authoritative 404 miss", second.Code, second.Header().Get(XCacheHeader)) + } + if got := forwards.Load(); got != 0 { + t.Errorf("tiered upstream forwards after missing local entry = %d, want 0", got) + } +} + +func TestTieredLargeShortBodyCleanupKeepsNewerSameETagPut(t *testing.T) { + body := cacheTestBodyAboveSmallObjectThreshold("cached body") + prefix := body[:shortCacheBodyPrefixSize] + cfg := config.NewDefault() + cfg.Mode = config.ModeTiered + cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SizeThreshold = int64(len(body) + 1) + if err := cfg.Validate(); err != nil { + t.Fatal(err) + } + + mem := cacheclient.NewMemoryCache() + bodyKey := "" + started := make(chan struct{}) + release := make(chan struct{}) + var releaseOnce sync.Once + releaseBody := func() { releaseOnce.Do(func() { close(release) }) } + defer releaseBody() + var firstRead atomic.Bool + client := &testStreamCacheClient{ + CacheClient: mem, + stream: func(ctx context.Context, key string, w io.Writer) error { + if key == bodyKey && !firstRead.Swap(true) { + _, err := w.Write(prefix) + close(started) + if err != nil { + return err + } + <-release + return nil + } + return mem.GetStream(ctx, key, w) + }, + } + store := cache.NewCacheWithClient(client, &cfg.Cache) + mock, forwards, _ := tieredMock() + svc := NewService(mock, store, cfg) + bucket, key := "bucket", "key" + + if w := tieredDo(t, svc, http.MethodPut, "/"+bucket+"/"+key, string(body), nil); w.Code != http.StatusOK { + t.Fatalf("initial local PUT status = %d", w.Code) + } + oldMeta, oldVersion, found, err := store.GetMetaWithVersion(context.Background(), bucket, key) + if err != nil || !found || oldMeta == nil || oldMeta.BodyRef == "" { + t.Fatalf("initial local metadata = %+v found=%v err=%v", oldMeta, found, err) + } + bodyKey = cache.MakeBodyKey(bucket, key, oldMeta.BodyRef) + if err := store.PutBodyStream(context.Background(), bucket, key, oldMeta.BodyRef, bytes.NewReader(prefix), 0); err != nil { + t.Fatalf("truncate local-tier test body: %v", err) + } + + first := httptest.NewRecorder() + firstResult := make(chan error, 1) + firstRequest := httptest.NewRequest(http.MethodGet, "/"+bucket+"/"+key, nil) + go func() { firstResult <- svc.HandleGetObject(first, firstRequest) }() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("local-tier GET did not commit its first cached chunk") + } + + if w := tieredDo(t, svc, http.MethodPut, "/"+bucket+"/"+key, string(body), nil); w.Code != http.StatusOK { + t.Fatalf("replacement local PUT status = %d", w.Code) + } + newMeta, newVersion, found, err := store.GetMetaWithVersion(context.Background(), bucket, key) + if err != nil || !found || newMeta == nil { + t.Fatalf("replacement metadata = %+v found=%v err=%v", newMeta, found, err) + } + if newVersion == oldVersion || newMeta.ETag != oldMeta.ETag || newMeta.BodyRef == oldMeta.BodyRef { + t.Fatalf("replacement version/ref/etag = %d/%q/%q, old = %d/%q/%q", newVersion, newMeta.BodyRef, newMeta.ETag, oldVersion, oldMeta.BodyRef, oldMeta.ETag) + } + + releaseBody() + if err := <-firstResult; err != nil { + t.Fatalf("first GET error = %v", err) + } + if first.Code != http.StatusOK || first.Header().Get(XCacheHeader) != XCacheHit || first.Header().Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(first.Body.Bytes(), prefix) { + t.Fatalf("first committed GET = %d headers=%v body_bytes=%d, want HIT with the short prefix", first.Code, first.Header(), first.Body.Len()) + } + if got := forwards.Load(); got != 0 { + t.Errorf("tiered upstream forwards = %d, want 0", got) + } + if _, currentVersion, found, err := store.GetMetaWithVersion(context.Background(), bucket, key); err != nil || !found || currentVersion != newVersion { + t.Fatalf("version-guarded cleanup lost replacement: found=%v version=%d want=%d err=%v", found, currentVersion, newVersion, err) + } + + second := tieredDo(t, svc, http.MethodGet, "/"+bucket+"/"+key, "", nil) + if second.Code != http.StatusOK || second.Header().Get(XCacheHeader) != XCacheHit || !bytes.Equal(second.Body.Bytes(), body) { + t.Fatalf("second GET = %d X-Cache=%q body_bytes=%d, want full replacement body", second.Code, second.Header().Get(XCacheHeader), second.Body.Len()) + } + if got := forwards.Load(); got != 0 { + t.Errorf("tiered upstream forwards after replacement GET = %d, want 0", got) + } +} + // A pre-existing content-addressed entry (proxy-written shape: body keyed by // ETag, BodyRef null) serves through the same engine unchanged — the // discriminator falls back to the ETag. From b8eaab3fa86c27e002c0de9ad4ff6bc119897119 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 11:54:17 +0000 Subject: [PATCH 09/13] fix(proxy): quarantine repeated body length mismatches Signed-off-by: Perfloop Agent --- README.md | 19 ++- cache/cache.go | 20 +++ cache/coordinator.go | 34 ++++ cache/coordinator_test.go | 29 ++++ cache/object.go | 18 ++- docs/architecture.md | 22 ++- docs/cache-control.md | 9 +- docs/tiered-mode.md | 17 +- proxy/cache_hit_stream_test.go | 276 +++++++++++++++++++++++++++++++++ proxy/caching.go | 14 +- proxy/localstore.go | 6 +- proxy/revalidation.go | 68 ++++++-- 12 files changed, 500 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 1ca4ac79..89e92ce6 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,24 @@ For production, TAG ships manifests, Compose files, and guides under [`deploy/`] 2. **Cache Hit**: Returns cached object with `X-Cache: HIT` header 3. **Cache Miss**: Forwards request to upstream Tigris, caches response, returns with `X-Cache: MISS` -For positive-length whole-object cache hits, TAG checks the cache stream length against metadata. In transparent and signing modes, a short buffered GET falls back to upstream before headers commit. If that fallback fails before upstream headers arrive, TAG returns the error without committing the cached prefix. A committed short or overlong stream is counted as a local error and is not followed by an upstream body. Proxy HEAD and metadata-based conditional 304 responses retain their existing behavior. Tiered local GETs have no upstream body fallback: a buffered length mismatch returns the authoritative `NoSuchKey` miss, while a committed short or overlong stream is counted as an error and is not replaced. Positive-length tiered HEAD and conditional 304 responses verify the cached body length before answering from metadata. Version-guarded cleanup removes the observed row in CAS mode. The default legacy proxy coordinator refuses versioned deletion, so a damaged row may remain until replaced or expired; each subsequent GET still checks its length. +For positive-length whole-object cache hits, TAG checks the cache stream length against metadata. In +transparent and signing modes, a short buffered GET falls back upstream before headers commit. If +that fallback fails before upstream headers arrive, TAG returns the error without committing the +cached prefix. A committed short or overlong stream is counted as a local error and is not followed +by an upstream body. + +Proxy HEAD and metadata-based conditional 304 responses retain their existing behavior. Tiered-local +GETs have no upstream body fallback: a buffered mismatch returns the authoritative `NoSuchKey` miss, +while a committed short or overlong stream is counted as an error and is not replaced. +Positive-length tiered HEAD and matching conditional 304 responses verify the body before answering +from metadata. + +CAS cleanup removes the metadata version observed by the request. The default legacy coordinator +cannot delete a specific version, so it keeps a TTL-bound hint keyed to the body and expected +length. Legacy whole-body reads check for the hint; while it remains present, a hinted body is +rechecked before streaming. If it is still incomplete, TAG falls back before commitment; a complete +same-ETag replacement remains usable. The metadata row may remain until TTL, older TAG nodes ignore +the hint, and an uncacheable origin response still requires an upstream fetch. ### Request Coalescing diff --git a/cache/cache.go b/cache/cache.go index d95d0cf5..ec094b2c 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -424,6 +424,26 @@ func (c *Cache) DeleteMetaIfVersion(ctx context.Context, bucket, key string, ver return c.coord.deleteMetaIfVersion(ctx, bucket, key, version) } +// MarkBodyLengthMismatch records an advisory, TTL-bound hint for a positive +// whole-body length mismatch. The selected coordinator decides whether it needs +// a hint; readers must revalidate the current body before treating it as absent. +func (c *Cache) MarkBodyLengthMismatch(ctx context.Context, bucket, key, discriminator string, length int64) error { + if !c.IsEnabled() || discriminator == "" || length <= 0 { + return nil + } + return c.coord.markBodyLengthMismatch(ctx, bucket, key, discriminator, length, c.defaultTTL) +} + +// HasBodyLengthMismatch reports whether the selected coordinator has a hint for +// this exact body identity and expected length. A positive result is not proof +// of corruption; callers recheck the current body before changing the response. +func (c *Cache) HasBodyLengthMismatch(ctx context.Context, bucket, key, discriminator string, length int64) (bool, error) { + if !c.IsEnabled() || discriminator == "" || length <= 0 { + return false, nil + } + return c.coord.hasBodyLengthMismatch(ctx, bucket, key, discriminator, length) +} + // DeleteIfETag invalidates the object's metadata only while it still carries // the given ETag, through the selected coordinator: an atomic versioned CAS // in CAS mode; the pre-CAS narrowed compare-then-delete in legacy mode. diff --git a/cache/coordinator.go b/cache/coordinator.go index d3a2bad0..8520571b 100644 --- a/cache/coordinator.go +++ b/cache/coordinator.go @@ -65,6 +65,11 @@ type metaCoordinator interface { // coordinator strength: legacy coordination stores no versions to compare // and refuses with (false, nil). deleteMetaIfVersion(ctx context.Context, bucket, key string, version uint64) (bool, error) + // Body-length hints are advisory and keyed separately from metadata. Legacy + // records them because it cannot delete a specific metadata version; CAS + // relies on its version-guarded delete and does not retain a hint. + markBodyLengthMismatch(ctx context.Context, bucket, key, discriminator string, length, ttl int64) error + hasBodyLengthMismatch(ctx context.Context, bucket, key, discriminator string, length int64) (bool, error) } // ============================================================================ @@ -216,6 +221,14 @@ func (c *casCoordinator) deleteMetaIfVersion(ctx context.Context, bucket, key st return true, nil } +func (c *casCoordinator) markBodyLengthMismatch(context.Context, string, string, string, int64, int64) error { + return nil +} + +func (c *casCoordinator) hasBodyLengthMismatch(context.Context, string, string, string, int64) (bool, error) { + return false, nil +} + // ============================================================================ // Legacy coordinator — timestamp tombstones, byte-faithful to v1.20 // ============================================================================ @@ -376,3 +389,24 @@ func (c *legacyCoordinator) tombstoneTimestamp(ctx context.Context, bucket, key func (c *legacyCoordinator) deleteMetaIfVersion(ctx context.Context, bucket, key string, version uint64) (bool, error) { return false, nil } + +// markBodyLengthMismatch keeps a TTL-bounded quarantine hint separate from the +// mutable metadata key. A reader must recheck the current body before acting on +// it, so a same-ETag replacement is not hidden by an old mismatch report. +func (c *legacyCoordinator) markBodyLengthMismatch(ctx context.Context, bucket, key, discriminator string, length, ttl int64) error { + if ttl <= 0 { + ttl = c.tombstoneTTL + } + return c.client.Put(ctx, makeBodyLengthMismatchKey(bucket, key, discriminator, length), []byte{1}, ttl) +} + +func (c *legacyCoordinator) hasBodyLengthMismatch(ctx context.Context, bucket, key, discriminator string, length int64) (bool, error) { + value, err := c.client.Get(ctx, makeBodyLengthMismatchKey(bucket, key, discriminator, length)) + if err != nil { + if isNotFoundError(err) { + return false, nil + } + return false, err + } + return len(value) > 0, nil +} diff --git a/cache/coordinator_test.go b/cache/coordinator_test.go index 764e1af3..974986c9 100644 --- a/cache/coordinator_test.go +++ b/cache/coordinator_test.go @@ -15,6 +15,35 @@ func newLegacyTestCache() (*Cache, *cacheclient.MemoryCache) { return NewCacheWithClient(mem, &cfg.Cache), mem } +// A legacy mismatch hint is scoped to the body identity and expected length. +// CAS relies on its versioned delete and does not retain a hint. +func TestLegacyBodyLengthMismatchHintIsLengthScoped(t *testing.T) { + ctx := context.Background() + legacy, _ := newLegacyTestCache() + if err := legacy.MarkBodyLengthMismatch(ctx, "b", "k", `"etag"`, 100); err != nil { + t.Fatalf("MarkBodyLengthMismatch: %v", err) + } + if found, err := legacy.HasBodyLengthMismatch(ctx, "b", "k", `"etag"`, 100); err != nil || !found { + t.Fatalf("legacy hint = (%v, %v), want (true, nil)", found, err) + } + if found, err := legacy.HasBodyLengthMismatch(ctx, "b", "k", `"etag"`, 101); err != nil || found { + t.Fatalf("different length hint = (%v, %v), want (false, nil)", found, err) + } + if found, err := legacy.HasBodyLengthMismatch(ctx, "b", "k", `"replacement"`, 100); err != nil || found { + t.Fatalf("different body hint = (%v, %v), want (false, nil)", found, err) + } + + cfg := config.NewDefault() + cfg.Cache.SetLegacyCoordination(false) + cas := NewCacheWithClient(cacheclient.NewMemoryCache(), &cfg.Cache) + if err := cas.MarkBodyLengthMismatch(ctx, "b", "k", `"etag"`, 100); err != nil { + t.Fatalf("CAS MarkBodyLengthMismatch: %v", err) + } + if found, err := cas.HasBodyLengthMismatch(ctx, "b", "k", `"etag"`, 100); err != nil || found { + t.Fatalf("CAS hint = (%v, %v), want (false, nil)", found, err) + } +} + // The default configuration selects the legacy coordinator: a fresh install or // an untouched upgrade must run the v1.20-compatible mechanism. func TestCoordinator_DefaultIsLegacy(t *testing.T) { diff --git a/cache/object.go b/cache/object.go index 99dbe6d2..32c937c3 100644 --- a/cache/object.go +++ b/cache/object.go @@ -14,11 +14,12 @@ import ( ) const ( - // Cache key prefixes for separate metadata and body storage - metaKeyPrefix = "meta|" - bodyKeyPrefix = "body|" - blockKeyPrefix = "blk|" - tombKeyPrefix = "tomb|" + // Cache key prefixes for metadata, bodies, blocks, tombstones, and mismatch hints. + metaKeyPrefix = "meta|" + bodyKeyPrefix = "body|" + blockKeyPrefix = "blk|" + tombKeyPrefix = "tomb|" + mismatchKeyPrefix = "length-mismatch|" ) // CachedObjectMeta represents cached S3 object metadata. @@ -415,6 +416,13 @@ func MakeBodyKey(bucket, key, etag string) string { return bodyKeyPrefix + bucket + "|" + key + "|" + etagKeyComponent(etag) } +// makeBodyLengthMismatchKey identifies a TTL-bound hint for one whole-body key +// and declared length. Readers still verify the current body before acting on +// the hint, so a replacement that now has the expected length remains usable. +func makeBodyLengthMismatchKey(bucket, key, discriminator string, length int64) string { + return mismatchKeyPrefix + MakeBodyKey(bucket, key, discriminator) + "|" + strconv.FormatInt(length, 10) +} + // MakeBlockKey creates the cache key for a single block of a block-mode object body // ("blk|bucket|key|||"). Blocks are ETag-scoped exactly like // whole bodies (MakeBodyKey), so a concurrent overwrite writes new block keys under the new diff --git a/docs/architecture.md b/docs/architecture.md index 8d98ba60..bdb44938 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -206,9 +206,25 @@ Client TAG Embedded Cache │ X-Cache: HIT │ │ ``` -For a positive-length whole-object cache entry, TAG checks the cache stream length against metadata before counting a local GET as successful. In transparent and signing modes, a short buffered GET falls back upstream before response headers commit. If upstream fails before headers arrive, TAG returns the error without committing the cached prefix. A large stream may already be committed when a short or overlong mismatch is detected; TAG records a local error and does not append upstream bytes. Version-guarded cleanup removes the observed metadata row in CAS mode. The default legacy proxy coordinator refuses versioned deletion, so a damaged row may remain until replacement or expiry; later GETs still check the length. Proxy HEAD and metadata-based conditional 304 responses retain their existing metadata behavior. - -Tiered local reads have no upstream body fallback. A buffered length mismatch returns the authoritative `NoSuchKey` miss without cached bytes; a committed short or overlong stream is recorded as a local error without an upstream append. Positive-length tiered HEAD and conditional 304 responses also verify the exact body length with a final-byte range probe before returning metadata. Zero-length and unknown-length objects retain their existing paths. Range and block-cache reads use separate helpers. +For a positive-length whole-object cache entry, TAG checks stream length against metadata before +counting a local GET as successful. In transparent and signing modes, a short buffered GET falls +back upstream before headers commit. If upstream fails before headers arrive, TAG returns the error +without committing the cached prefix. A committed short or overlong stream is recorded as a local +error; no upstream body is appended. Proxy HEAD and metadata-based conditional 304 responses retain +their existing metadata behavior. + +Tiered-local GETs have no upstream body fallback. A buffered mismatch returns the authoritative +`NoSuchKey` miss without cached bytes; a committed mismatch is recorded as a local error without an +upstream append. Positive-length tiered HEAD and matching conditional 304 responses verify body +length with a final-byte range probe before returning metadata. Zero-length and unknown-length +objects retain their existing paths. Range body reads use separate helpers. + +CAS cleanup removes the metadata version observed by the request. Legacy coordination cannot safely +delete that row by version, so it stores a TTL-bound hint keyed by body identity and expected +length. While the hint remains present, updated legacy whole-body readers look it up and range-check +flagged bodies before streaming. A still-incomplete entry falls back before commitment, while an +exact same-ETag replacement remains usable. The metadata row may remain until TTL; older TAG nodes +ignore the hint, and an uncacheable origin response still requires an upstream fetch. ### GET Object (Cache Miss) diff --git a/docs/cache-control.md b/docs/cache-control.md index 83df4b5d..ee40c863 100644 --- a/docs/cache-control.md +++ b/docs/cache-control.md @@ -28,7 +28,14 @@ curl -H "Cache-Control: no-cache" -H "Range: bytes=0-99" \ http://localhost:8080/my-bucket/my-key ``` -If the revalidation request to upstream fails, TAG serves the stale cached copy as a fallback. +In proxy modes, a failed revalidation tries to serve stale cached content. A body-bearing +positive-length whole-object response is complete only when its cache body matches metadata length; +proxy HEAD and metadata-only conditional 304 responses retain their validator-based behavior. + +A short body or other cache error found before headers commit triggers a fresh upstream request; if +that request also fails, TAG returns the error. If a length mismatch or stream error is found after +large-response headers commit, TAG records an incomplete response as an error and does not append an +upstream body. Range body reads use their separate range path. ## Bypass Cache diff --git a/docs/tiered-mode.md b/docs/tiered-mode.md index f2e234a0..0c9c5899 100644 --- a/docs/tiered-mode.md +++ b/docs/tiered-mode.md @@ -17,11 +17,22 @@ memory, so the threshold can be sized to the workload without a per-request memory cost. The MD5 ETag is computed over the decoded bytes as they stream; because the ETag cannot exist before the last byte, the body is keyed by a per-write id carried in the metadata (`BodyRef`), and the metadata commit is -what makes the entry visible. `If-Match`/`If-None-Match` are honored; GET, -HEAD, and DELETE are served entirely locally. Reads and writes of small +what makes the entry visible. `If-Match`/`If-None-Match` are honored for a local entry whose body is +present; GET, HEAD, and DELETE are served locally. Reads and writes of small objects generate zero upstream traffic. -For a positive-length whole-object local hit, TAG checks the cache stream length against metadata. A buffered length mismatch found before GET headers commit is discarded; TAG attempts version-guarded metadata cleanup, records a local request error, and returns the authoritative `NoSuchKey` miss so the caller can fall back to its system of record. If a large local body is found short or overlong after streaming commits headers, the response cannot be replaced: TAG records a local request error, attempts version-guarded metadata cleanup, and does not append an upstream body. Cleanup is best effort. Positive-length HEAD and conditional 304 responses also verify the exact body length with a final-byte range probe before answering from metadata; a confirmed mismatch returns the authoritative `NoSuchKey` miss instead of implying that the local body is servable. +For a positive-length whole-object local hit, TAG checks the cache stream length against metadata. A +present but short or overlong buffered body is discarded; TAG attempts version-guarded metadata +cleanup, records a local request error, and returns the authoritative `NoSuchKey` miss so the caller +can fall back to its system of record. A body absent at the existing first-byte probe continues to +follow the ordinary `NoSuchKey` miss path. + +If a large local body is found short or overlong after streaming commits headers, the response +cannot be replaced. TAG records a local request error, attempts version-guarded cleanup, and does +not append an upstream body. Cleanup is best effort. Positive-length HEAD and matching conditional +304 responses also verify the body length with a final-byte range probe before answering from +metadata; a confirmed mismatch returns `NoSuchKey` instead of implying that the local body is +servable. **Large objects are the upstream tier.** The PUT passes through to upstream and TAG stores a metadata marker locally. HEAD answers from the marker; GET diff --git a/proxy/cache_hit_stream_test.go b/proxy/cache_hit_stream_test.go index 517123f8..c7db91b4 100644 --- a/proxy/cache_hit_stream_test.go +++ b/proxy/cache_hit_stream_test.go @@ -523,6 +523,159 @@ func TestHandleGetObject_ShortBodyCleanupPreservesSameETagReplacement(t *testing } } +func TestHandleGetObject_EmptyCacheBodyPreservesSameETagReplacement(t *testing.T) { + body := cacheTestBodyAboveSmallObjectThreshold("cached body") + for _, tc := range []struct { + name string + legacy bool + revalidate bool + }{ + {name: "legacy default", legacy: true}, + {name: "CAS coordinator", legacy: false}, + {name: "legacy revalidation fallback", legacy: true, revalidate: true}, + } { + t.Run(tc.name, func(t *testing.T) { + started := make(chan struct{}) + release := make(chan struct{}) + var releaseOnce sync.Once + releaseStream := func() { releaseOnce.Do(func() { close(release) }) } + defer releaseStream() + cfg := config.NewDefault() + cfg.Cache.SetBlockCachingEnabled(false) + cfg.Cache.SetLegacyCoordination(tc.legacy) + base := cacheclient.NewMemoryCache() + var streamOnce sync.Once + stream := func(ctx context.Context, key string, w io.Writer) error { + first := false + var streamErr error + streamOnce.Do(func() { + first = true + close(started) + select { + case <-release: + case <-ctx.Done(): + streamErr = ctx.Err() + } + }) + if first { + return streamErr + } + return base.GetStream(ctx, key, w) + } + client := &testStreamCacheClient{CacheClient: base, stream: stream} + store := cache.NewCacheWithClient(client, &cfg.Cache) + meta := &cache.CachedObjectMeta{ + Bucket: "bucket", + Key: "key", + ETag: `"etag"`, + ContentType: "text/plain", + ContentLength: int64(len(body)), + StatusCode: http.StatusOK, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, body, 0); err != nil { + t.Fatalf("seed cache: %v", err) + } + svc := NewService(&mockForwarder{}, store, cfg) + var upstreamCalls atomic.Int32 + forwarder := &mockForwarder{ + doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { + upstreamCalls.Add(1) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{ + "Content-Length": []string{strconv.Itoa(len(body))}, + "Cache-Control": []string{"no-store"}, + "Content-Type": []string{"text/plain"}, + "ETag": []string{meta.ETag}, + }, + Body: io.NopCloser(bytes.NewReader(body)), + ContentLength: int64(len(body)), + }, nil + }, + } + if tc.revalidate { + forwarder.conditionalResp = &http.Response{ + StatusCode: http.StatusNotModified, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(nil)), + } + forwarder.forwardFunc = func(_ context.Context, w http.ResponseWriter, _ *http.Request) error { + upstreamCalls.Add(1) + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("Content-Type", "text/plain") + w.Header().Set("ETag", meta.ETag) + w.WriteHeader(http.StatusOK) + _, err := w.Write(body) + return err + } + } + svc.forwarder = forwarder + w := httptest.NewRecorder() + done := make(chan error, 1) + go func() { + r := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + if tc.revalidate { + r.Header.Set("Cache-Control", "no-cache") + } + done <- svc.HandleGetObject(w, r) + }() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("empty cache stream did not start") + } + + replacement := &cache.CachedObjectMeta{ + Bucket: meta.Bucket, + Key: meta.Key, + ETag: meta.ETag, + ContentType: meta.ContentType, + ContentLength: meta.ContentLength, + StatusCode: meta.StatusCode, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, replacement, body, 0); err != nil { + t.Fatalf("commit same-ETag replacement: %v", err) + } + releaseStream() + if err := <-done; err != nil { + t.Fatalf("HandleGetObject() error = %v, want successful upstream fallback", err) + } + if tc.revalidate && !forwarder.conditionalCalled { + t.Fatal("empty-body fallback did not take the revalidation path") + } + if w.Code != http.StatusOK || w.Header().Get(XCacheHeader) != XCacheMiss || w.Header().Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(w.Body.Bytes(), body) { + t.Fatalf("empty-body fallback = %d headers=%v body_bytes=%d, want full MISS", w.Code, w.Header(), w.Body.Len()) + } + if got := upstreamCalls.Load(); got != 1 { + t.Fatalf("empty-body fallback upstream calls = %d, want 1", got) + } + + current, found, err := store.GetMeta(context.Background(), meta.Bucket, meta.Key) + if err != nil || !found || current == nil || current.ETag != meta.ETag { + t.Fatalf("same-ETag replacement metadata was removed: current=%+v found=%v err=%v", current, found, err) + } + var currentBody bytes.Buffer + if err := store.GetBodyStream(context.Background(), meta.Bucket, meta.Key, current.BodyDiscriminator(), ¤tBody); err != nil { + t.Fatalf("read replacement body: %v", err) + } + if !bytes.Equal(currentBody.Bytes(), body) { + t.Errorf("replacement body length = %d, want %d", currentBody.Len(), len(body)) + } + second := httptest.NewRecorder() + if err := svc.HandleGetObject(second, httptest.NewRequest(http.MethodGet, "/bucket/key", nil)); err != nil { + t.Fatalf("second GET error = %v", err) + } + if second.Code != http.StatusOK || second.Header().Get(XCacheHeader) != XCacheHit || !bytes.Equal(second.Body.Bytes(), body) { + t.Errorf("replacement GET = %d X-Cache=%q body_bytes=%d, want full HIT", second.Code, second.Header().Get(XCacheHeader), second.Body.Len()) + } + if got := upstreamCalls.Load(); got != 1 { + t.Errorf("replacement GET caused %d upstream calls, want 1", got) + } + }) + } +} + func TestHandleGetObject_LargeShortCacheBodyInvalidatesBeforeNextGetInCAS(t *testing.T) { body := cacheTestBodyAboveSmallObjectThreshold("cached body") prefix := body[:shortCacheBodyPrefixSize] @@ -653,6 +806,129 @@ func TestHandleGetObject_RevalidationLargeShortBodyInvalidatesMetadataInCAS(t *t } } +func TestLegacyOverlongHintPreventsIncompleteReplay(t *testing.T) { + body := cacheTestBodyAboveSmallObjectThreshold("cached body") + overlong := append(append([]byte(nil), body...), []byte(" extra")...) + cfg := config.NewDefault() + cfg.Cache.SetBlockCachingEnabled(false) + base := cacheclient.NewMemoryCache() + var bodyStreams atomic.Int32 + var rangeReads atomic.Int32 + client := &testStreamCacheClient{ + CacheClient: base, + stream: func(ctx context.Context, key string, w io.Writer) error { + bodyStreams.Add(1) + return base.GetStream(ctx, key, w) + }, + rangeStream: func(ctx context.Context, key string, start, end int64, w io.Writer) error { + rangeReads.Add(1) + return base.GetRangeStream(ctx, key, start, end, w) + }, + } + store := cache.NewCacheWithClient(client, &cfg.Cache) + meta := &cache.CachedObjectMeta{ + Bucket: "bucket", + Key: "key", + ETag: `"etag"`, + ContentType: "text/plain", + ContentLength: int64(len(body)), + StatusCode: http.StatusOK, + } + if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, overlong, 0); err != nil { + t.Fatalf("seed overlong legacy entry: %v", err) + } + svc := NewService(&mockForwarder{}, store, cfg) + var upstreamCalls atomic.Int32 + forwarder := &mockForwarder{ + conditionalResp: &http.Response{ + StatusCode: http.StatusNotModified, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(nil)), + }, + forwardFunc: func(_ context.Context, w http.ResponseWriter, _ *http.Request) error { + upstreamCalls.Add(1) + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("Content-Type", "text/plain") + w.Header().Set("ETag", meta.ETag) + w.WriteHeader(http.StatusOK) + _, err := w.Write(body) + return err + }, + } + svc.forwarder = forwarder + + beforeErrors := requestCountFrom(t, "error", metrics.SourceLocal) + beforeSuccesses := requestCountFrom(t, "success", metrics.SourceLocal) + handlerErrors := make(chan error, 2) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handlerErrors <- svc.HandleGetObject(w, r) + })) + defer server.Close() + clientHTTP := &http.Client{Timeout: 5 * time.Second} + + first, err := clientHTTP.Get(server.URL + "/bucket/key") + if err != nil { + t.Fatalf("first GET: %v", err) + } + firstBody, firstReadErr := io.ReadAll(first.Body) + _ = first.Body.Close() + if first.StatusCode != http.StatusOK || first.Header.Get("Content-Length") != strconv.Itoa(len(body)) { + t.Fatalf("first response = %d headers=%v, want 200 with advertised length", first.StatusCode, first.Header) + } + if len(firstBody) != 0 || !errors.Is(firstReadErr, io.ErrUnexpectedEOF) { + t.Fatalf("first response read = %d bytes, %v; want empty body and unexpected EOF", len(firstBody), firstReadErr) + } + if err := <-handlerErrors; err != nil { + t.Fatalf("first HandleGetObject: %v", err) + } + if got := bodyStreams.Load(); got != 1 { + t.Fatalf("cache body streams after first GET = %d, want 1", got) + } + if got := requestCountFrom(t, "error", metrics.SourceLocal); got != beforeErrors+1 { + t.Errorf("first local error count delta = %v, want 1", got-beforeErrors) + } + if got := requestCountFrom(t, "success", metrics.SourceLocal); got != beforeSuccesses { + t.Errorf("first local success count delta = %v, want 0", got-beforeSuccesses) + } + if hinted, err := store.HasBodyLengthMismatch(context.Background(), meta.Bucket, meta.Key, meta.BodyDiscriminator(), meta.ContentLength); err != nil || !hinted { + t.Fatalf("legacy length hint = (%v, %v), want (true, nil)", hinted, err) + } + + secondRequest, err := http.NewRequest(http.MethodGet, server.URL+"/bucket/key", nil) + if err != nil { + t.Fatalf("second request: %v", err) + } + secondRequest.Header.Set("Cache-Control", "no-cache") + second, err := clientHTTP.Do(secondRequest) + if err != nil { + t.Fatalf("second revalidation GET: %v", err) + } + secondBody, secondReadErr := io.ReadAll(second.Body) + _ = second.Body.Close() + if err := <-handlerErrors; err != nil { + t.Fatalf("second revalidation HandleGetObject: %v", err) + } + if !forwarder.conditionalCalled { + t.Fatal("second GET did not take the revalidation path") + } + if secondReadErr != nil || second.StatusCode != http.StatusOK || second.Header.Get(XCacheHeader) != XCacheMiss || second.Header.Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(secondBody, body) { + t.Fatalf("second revalidation response = %d headers=%v body_bytes=%d read_error=%v, want complete 200 MISS", second.StatusCode, second.Header, len(secondBody), secondReadErr) + } + if got := bodyStreams.Load(); got != 1 { + t.Errorf("cache body streams after quarantined retry = %d, want 1", got) + } + if got := rangeReads.Load(); got != 1 { + t.Errorf("range probes after quarantined retry = %d, want 1", got) + } + if got := upstreamCalls.Load(); got != 1 { + t.Errorf("upstream calls after quarantined retry = %d, want 1", got) + } + if _, found, err := store.GetMeta(context.Background(), meta.Bucket, meta.Key); err != nil || !found { + t.Errorf("legacy metadata should remain until TTL: found=%v err=%v", found, err) + } +} + func TestHandleGetObject_ShortCacheBodyUpstreamFailureBeforeCommit(t *testing.T) { objectBody := []byte("complete object body") cacheBody := objectBody[:5] diff --git a/proxy/caching.go b/proxy/caching.go index 9848eec9..79b482b6 100644 --- a/proxy/caching.go +++ b/proxy/caching.go @@ -83,13 +83,17 @@ var errCachePopulateDeclined = errors.New("cache populate declined: concurrent w // probably fine and we merely could not finish reading it — overwhelmingly a // canceled context from a client that disconnected mid-stream (in cluster mode the // read is routed over gRPC, so it can arrive as a status code instead of a plain -// context error), or a briefly unreachable peer. Those leave the entry alone. +// context error), a briefly unreachable peer, or an unavailable legacy length hint. +// Those leave the entry alone. // Anything else heals the entry; the worst case is one extra miss on a rare I/O // error, which repopulates immediately — a far better trade than a 24h loop. func bodyGone(err error) bool { if err == nil { return false } + if errors.Is(err, errCacheBodyLengthHintUnavailable) { + return false + } if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return false } @@ -127,6 +131,14 @@ func (s *Service) bodyLengthMatches(ctx context.Context, bucket, key string, met return cw.written == 1, nil } +func (s *Service) rememberBodyLengthMismatch(ctx context.Context, bucket, key string, meta *cache.CachedObjectMeta) { + if err := s.cache.MarkBodyLengthMismatch(ctx, bucket, key, meta.BodyDiscriminator(), meta.ContentLength); err != nil { + log.Warn().Err(err).Str("bucket", bucket).Str("key", key). + Int64("expected_bytes", meta.ContentLength). + Msg("Failed to retain cache body length mismatch hint") + } +} + // cacheWriteTimeoutForSize returns a timeout scaled to contentLength. // Returns at least cacheWriteTimeout (60s), scaling up for large objects. func cacheWriteTimeoutForSize(contentLength int64) time.Duration { diff --git a/proxy/localstore.go b/proxy/localstore.go index 30f61c19..57908c34 100644 --- a/proxy/localstore.go +++ b/proxy/localstore.go @@ -120,9 +120,9 @@ func (s *Service) serveOriginlessObject(w http.ResponseWriter, r *http.Request, return s.originlessMiss(w, r, operation, start) } - // Preserve S3 precondition precedence before checking body length. A failed - // If-Match/If-Unmodified-Since answers 412 from metadata even when the body - // itself is short; it does not claim that the body can be served. + // For a body that passed the first-byte presence check, preserve S3 + // precondition precedence before checking the full length. A failed If-Match + // or If-Unmodified-Since answers 412 from metadata even if that body is short. if writePreconditionFailed(w, r, meta) { metrics.RecordRequest(operation, "success", metrics.SourceLocal, time.Since(start).Seconds()) return nil diff --git a/proxy/revalidation.go b/proxy/revalidation.go index b1fbb20a..84f8c05d 100644 --- a/proxy/revalidation.go +++ b/proxy/revalidation.go @@ -15,7 +15,10 @@ import ( "github.com/tigrisdata/tag/metrics" ) -var errCacheBodyLengthMismatch = errors.New("cache body length mismatch") +var ( + errCacheBodyLengthMismatch = errors.New("cache body length mismatch") + errCacheBodyLengthHintUnavailable = errors.New("cache body length hint unavailable") +) // revalidateAndServe sends a conditional GET to upstream and serves the result. // Supports both full-object and range requests. For range requests, the Range header @@ -320,9 +323,10 @@ func (s *Service) handleRevalidation200( return copyErr } -// serveFromCache serves a whole object from the cache body. Buffered length -// mismatches return before headers commit; a mismatch found after a streamed -// response commits is accounted locally and never triggers an upstream append. +// serveFromCache serves a whole object from the cache body. A retained legacy +// length hint rechecks the current body before streaming. Buffered mismatches +// return before headers commit; committed mismatches are accounted locally and +// never trigger an upstream append. func (s *Service) serveFromCache( ctx context.Context, w http.ResponseWriter, @@ -337,29 +341,55 @@ func (s *Service) serveFromCache( return nil } + // A legacy mismatch hint is only a reason to recheck, never proof that a + // same-ETag replacement is bad. Keep the check at the shared whole-body + // serve boundary so normal, revalidated, and stale reads take the same path. + if meta.ContentLength > 0 { + hinted, err := s.cache.HasBodyLengthMismatch(ctx, bucket, key, meta.BodyDiscriminator(), meta.ContentLength) + if err != nil { + return fmt.Errorf("%w: %w", errCacheBodyLengthHintUnavailable, err) + } + if hinted { + matches, err := s.bodyLengthMatches(ctx, bucket, key, meta) + if err != nil { + return fmt.Errorf("%w: %w", errCacheBodyLengthHintUnavailable, err) + } + if !matches { + s.rememberBodyLengthMismatch(ctx, bucket, key, meta) + if invalidateOnLengthMismatch != nil { + invalidateOnLengthMismatch() + } + return fmt.Errorf("%w for %s/%s: quarantined body still differs from metadata length %d", errCacheBodyLengthMismatch, bucket, key, meta.ContentLength) + } + } + } + // Small objects: buffer and verify before committing the response. if meta.ContentLength <= smallObjectThreshold { bodyBuf := bufferPool.Get().(*bytes.Buffer) bodyBuf.Reset() bodyErr := s.cache.GetBodyStream(ctx, bucket, key, meta.BodyDiscriminator(), bodyBuf) - if bodyErr == nil && bodyBuf.Len() > 0 { - if meta.ContentLength > 0 && int64(bodyBuf.Len()) != meta.ContentLength { - bodyLen := bodyBuf.Len() + if bodyErr == nil { + bodyLen := int64(bodyBuf.Len()) + if meta.ContentLength > 0 && bodyLen != meta.ContentLength { putBuffer(bodyBuf) + s.rememberBodyLengthMismatch(ctx, bucket, key, meta) if invalidateOnLengthMismatch != nil { invalidateOnLengthMismatch() } return fmt.Errorf("%w for %s/%s: got %d bytes, want %d", errCacheBodyLengthMismatch, bucket, key, bodyLen, meta.ContentLength) } - meta.WriteHeaders(w) - writeCacheStatus(w, XCacheHit) - w.WriteHeader(meta.StatusCode) - n, _ := w.Write(bodyBuf.Bytes()) - metrics.BytesTransferred.WithLabelValues("out").Add(float64(n)) - metrics.RecordRequest("GetObject", "success", metrics.SourceLocal, time.Since(start).Seconds()) - putBuffer(bodyBuf) - return nil + if bodyLen > 0 { + meta.WriteHeaders(w) + writeCacheStatus(w, XCacheHit) + w.WriteHeader(meta.StatusCode) + n, _ := w.Write(bodyBuf.Bytes()) + metrics.BytesTransferred.WithLabelValues("out").Add(float64(n)) + metrics.RecordRequest("GetObject", "success", metrics.SourceLocal, time.Since(start).Seconds()) + putBuffer(bodyBuf) + return nil + } } putBuffer(bodyBuf) // Body unavailable — return error (caller may fall through to upstream) @@ -379,6 +409,13 @@ func (s *Service) serveFromCache( return fmt.Errorf("cache body unavailable: %w", bodyErr) } if !cw.committed { + if bodyErr == nil && meta.ContentLength > 0 { + s.rememberBodyLengthMismatch(ctx, bucket, key, meta) + if invalidateOnLengthMismatch != nil { + invalidateOnLengthMismatch() + } + return fmt.Errorf("%w for %s/%s: got 0 bytes, want %d", errCacheBodyLengthMismatch, bucket, key, meta.ContentLength) + } return fmt.Errorf("cache body empty for %s/%s", bucket, key) } @@ -392,6 +429,7 @@ func (s *Service) serveFromCache( Int64("written_bytes", cw.written). Int64("offered_bytes", cw.offered). Msg("Cache body length mismatch after headers committed") + s.rememberBodyLengthMismatch(ctx, bucket, key, meta) if invalidateOnLengthMismatch != nil { invalidateOnLengthMismatch() } From d314b6e71807880cb0b127524f3229760130f980 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 12:28:00 +0000 Subject: [PATCH 10/13] test(proxy): remove unused cache reader helper Signed-off-by: Perfloop Agent --- proxy/cache_hit_stream_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/proxy/cache_hit_stream_test.go b/proxy/cache_hit_stream_test.go index c7db91b4..e7a94b50 100644 --- a/proxy/cache_hit_stream_test.go +++ b/proxy/cache_hit_stream_test.go @@ -26,19 +26,6 @@ type testStreamCacheClient struct { rangeStream func(context.Context, string, int64, int64, io.Writer) error } -type originBodyReader struct { - io.ReadCloser - onError func(error) -} - -func (r *originBodyReader) Read(p []byte) (int, error) { - n, err := r.ReadCloser.Read(p) - if err != nil && !errors.Is(err, io.EOF) && r.onError != nil { - r.onError(err) - } - return n, err -} - type commitTrackingResponseWriter struct { *httptest.ResponseRecorder committed bool From 9eb40895c5992e790b6542915dac48bc9a8a719f Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 12:56:59 +0000 Subject: [PATCH 11/13] test(proxy): retain legacy stream-error coverage Signed-off-by: Perfloop Agent --- proxy/cache_hit_stream_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/cache_hit_stream_test.go b/proxy/cache_hit_stream_test.go index e7a94b50..fcae74f6 100644 --- a/proxy/cache_hit_stream_test.go +++ b/proxy/cache_hit_stream_test.go @@ -268,7 +268,7 @@ func TestHandleGetObject_LargeCacheHitPostCommitErrorDoesNotFallBack(t *testing. } return streamErr } - svc, _, _ := newLargeStreamCacheServiceWithCoordinator(t, body, stream, false) + svc, _, _ := newLargeStreamCacheService(t, body, stream) var upstreamCalls atomic.Int32 svc.forwarder = &mockForwarder{ From c33f5a7fd548c53d959a1b9cdca6aaa31d8cb708 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 13:46:12 +0000 Subject: [PATCH 12/13] test(proxy): consolidate cache fixture helpers Signed-off-by: Perfloop Agent --- proxy/cache_body_length_test.go | 22 +--------------------- proxy/cache_hit_stream_test.go | 8 ++------ 2 files changed, 3 insertions(+), 27 deletions(-) diff --git a/proxy/cache_body_length_test.go b/proxy/cache_body_length_test.go index a5c04a97..404e369c 100644 --- a/proxy/cache_body_length_test.go +++ b/proxy/cache_body_length_test.go @@ -26,26 +26,6 @@ func cacheTestBodyAboveSmallObjectThreshold(pattern string) []byte { return bytes.Repeat(patternBytes, smallObjectThreshold/len(patternBytes)+1) } -func newWholeObjectCacheService(t *testing.T, objectBody, cacheBody []byte) (*Service, *cache.CachedObjectMeta) { - t.Helper() - - cfg := config.NewDefault() - cfg.Cache.SetBlockCachingEnabled(false) - store := cache.NewCacheWithClient(cacheclient.NewMemoryCache(), &cfg.Cache) - meta := &cache.CachedObjectMeta{ - Bucket: "bucket", - Key: "key", - ETag: `"etag"`, - ContentType: "text/plain", - ContentLength: int64(len(objectBody)), - StatusCode: http.StatusOK, - } - if err := store.PutWithMeta(context.Background(), meta.Bucket, meta.Key, meta, cacheBody, 0); err != nil { - t.Fatalf("seed whole-object cache: %v", err) - } - return NewService(&mockForwarder{}, store, cfg), meta -} - func requestCountForOperation(t *testing.T, operation, status, source string) float64 { t.Helper() @@ -159,7 +139,7 @@ func TestHandleGetObject_CacheBodyLength(t *testing.T) { } for _, tt := range proxyCases { t.Run(tt.name, func(t *testing.T) { - svc, _ := newWholeObjectCacheService(t, tt.objectBody, tt.cacheBody) + svc, _ := newWholeObjectCacheServiceWithCoordinator(t, tt.objectBody, tt.cacheBody, true) var upstreamCalls atomic.Int32 svc.forwarder = &mockForwarder{ doRequestFunc: func(context.Context, *http.Request, string, string) (*http.Response, error) { diff --git a/proxy/cache_hit_stream_test.go b/proxy/cache_hit_stream_test.go index fcae74f6..3bde8c8e 100644 --- a/proxy/cache_hit_stream_test.go +++ b/proxy/cache_hit_stream_test.go @@ -58,15 +58,11 @@ func (c *testStreamCacheClient) GetRangeStream(ctx context.Context, key string, } func newLargeStreamCacheService(t *testing.T, body []byte, stream func(context.Context, string, io.Writer) error) (*Service, *cache.Cache, *cache.CachedObjectMeta) { - return newLargeStreamCacheServiceWithCoordinator(t, body, stream, true) -} - -func newLargeStreamCacheServiceWithCoordinator(t *testing.T, body []byte, stream func(context.Context, string, io.Writer) error, legacy bool) (*Service, *cache.Cache, *cache.CachedObjectMeta) { t.Helper() cfg := config.NewDefault() cfg.Cache.SetBlockCachingEnabled(false) - cfg.Cache.SetLegacyCoordination(legacy) + cfg.Cache.SetLegacyCoordination(true) base := cacheclient.NewMemoryCache() client := &testStreamCacheClient{CacheClient: base, stream: stream} store := cache.NewCacheWithClient(client, &cfg.Cache) @@ -919,7 +915,7 @@ func TestLegacyOverlongHintPreventsIncompleteReplay(t *testing.T) { func TestHandleGetObject_ShortCacheBodyUpstreamFailureBeforeCommit(t *testing.T) { objectBody := []byte("complete object body") cacheBody := objectBody[:5] - svc, _ := newWholeObjectCacheService(t, objectBody, cacheBody) + svc, _ := newWholeObjectCacheServiceWithCoordinator(t, objectBody, cacheBody, true) wantErr := errors.New("upstream request failed") var upstreamCalls atomic.Int32 svc.forwarder = &mockForwarder{ From ff287a74e4e2b655ae24737b88aec989ccf92022 Mon Sep 17 00:00:00 2001 From: Perfloop Agent Date: Wed, 14 Oct 2026 14:14:45 +0000 Subject: [PATCH 13/13] fix(proxy): guard short unexpected EOF cleanup Signed-off-by: Perfloop Agent --- proxy/cache_hit_stream_test.go | 50 ++++++++++++++++++++++++---------- proxy/revalidation.go | 17 +++++++++++- 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/proxy/cache_hit_stream_test.go b/proxy/cache_hit_stream_test.go index 3bde8c8e..659f27f2 100644 --- a/proxy/cache_hit_stream_test.go +++ b/proxy/cache_hit_stream_test.go @@ -507,17 +507,27 @@ func TestHandleGetObject_ShortBodyCleanupPreservesSameETagReplacement(t *testing } func TestHandleGetObject_EmptyCacheBodyPreservesSameETagReplacement(t *testing.T) { - body := cacheTestBodyAboveSmallObjectThreshold("cached body") + largeBody := cacheTestBodyAboveSmallObjectThreshold("cached body") + shortBody := []byte("cached replacement body") for _, tc := range []struct { - name string - legacy bool - revalidate bool + name string + legacy bool + revalidate bool + stale bool + unexpectedEOF bool + prefixBytes int + body []byte }{ - {name: "legacy default", legacy: true}, - {name: "CAS coordinator", legacy: false}, - {name: "legacy revalidation fallback", legacy: true, revalidate: true}, + {name: "legacy default", legacy: true, body: largeBody}, + {name: "CAS coordinator", legacy: false, body: largeBody}, + {name: "legacy revalidation fallback", legacy: true, revalidate: true, body: largeBody}, + {name: "legacy short read error", legacy: true, unexpectedEOF: true, prefixBytes: 1, body: shortBody}, + {name: "legacy large read error before commit", legacy: true, unexpectedEOF: true, body: largeBody}, + {name: "legacy revalidation short read error", legacy: true, revalidate: true, unexpectedEOF: true, prefixBytes: 1, body: shortBody}, + {name: "legacy stale fallback short read error", legacy: true, revalidate: true, stale: true, unexpectedEOF: true, prefixBytes: 1, body: shortBody}, } { t.Run(tc.name, func(t *testing.T) { + body := tc.body started := make(chan struct{}) release := make(chan struct{}) var releaseOnce sync.Once @@ -533,12 +543,18 @@ func TestHandleGetObject_EmptyCacheBodyPreservesSameETagReplacement(t *testing.T var streamErr error streamOnce.Do(func() { first = true + if tc.prefixBytes > 0 { + _, streamErr = w.Write(body[:tc.prefixBytes]) + } close(started) select { case <-release: case <-ctx.Done(): streamErr = ctx.Err() } + if tc.unexpectedEOF && streamErr == nil { + streamErr = io.ErrUnexpectedEOF + } }) if first { return streamErr @@ -577,10 +593,14 @@ func TestHandleGetObject_EmptyCacheBodyPreservesSameETagReplacement(t *testing.T }, } if tc.revalidate { - forwarder.conditionalResp = &http.Response{ - StatusCode: http.StatusNotModified, - Header: make(http.Header), - Body: io.NopCloser(bytes.NewReader(nil)), + if tc.stale { + forwarder.conditionalErr = errors.New("upstream revalidation failed") + } else { + forwarder.conditionalResp = &http.Response{ + StatusCode: http.StatusNotModified, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(nil)), + } } forwarder.forwardFunc = func(_ context.Context, w http.ResponseWriter, _ *http.Request) error { upstreamCalls.Add(1) @@ -606,7 +626,7 @@ func TestHandleGetObject_EmptyCacheBodyPreservesSameETagReplacement(t *testing.T select { case <-started: case <-time.After(2 * time.Second): - t.Fatal("empty cache stream did not start") + t.Fatal("cache stream did not start") } replacement := &cache.CachedObjectMeta{ @@ -625,13 +645,13 @@ func TestHandleGetObject_EmptyCacheBodyPreservesSameETagReplacement(t *testing.T t.Fatalf("HandleGetObject() error = %v, want successful upstream fallback", err) } if tc.revalidate && !forwarder.conditionalCalled { - t.Fatal("empty-body fallback did not take the revalidation path") + t.Fatal("cache-body fallback did not take the revalidation path") } if w.Code != http.StatusOK || w.Header().Get(XCacheHeader) != XCacheMiss || w.Header().Get("Content-Length") != strconv.Itoa(len(body)) || !bytes.Equal(w.Body.Bytes(), body) { - t.Fatalf("empty-body fallback = %d headers=%v body_bytes=%d, want full MISS", w.Code, w.Header(), w.Body.Len()) + t.Fatalf("cache-body fallback = %d headers=%v body_bytes=%d, want full MISS", w.Code, w.Header(), w.Body.Len()) } if got := upstreamCalls.Load(); got != 1 { - t.Fatalf("empty-body fallback upstream calls = %d, want 1", got) + t.Fatalf("cache-body fallback upstream calls = %d, want 1", got) } current, found, err := store.GetMeta(context.Background(), meta.Bucket, meta.Key) diff --git a/proxy/revalidation.go b/proxy/revalidation.go index 84f8c05d..b286bfc3 100644 --- a/proxy/revalidation.go +++ b/proxy/revalidation.go @@ -370,8 +370,16 @@ func (s *Service) serveFromCache( bodyBuf.Reset() bodyErr := s.cache.GetBodyStream(ctx, bucket, key, meta.BodyDiscriminator(), bodyBuf) + bodyLen := int64(bodyBuf.Len()) + if bodyErr != nil && meta.ContentLength > 0 && bodyLen < meta.ContentLength && errors.Is(bodyErr, io.ErrUnexpectedEOF) { + putBuffer(bodyBuf) + s.rememberBodyLengthMismatch(ctx, bucket, key, meta) + if invalidateOnLengthMismatch != nil { + invalidateOnLengthMismatch() + } + return fmt.Errorf("%w for %s/%s: got %d bytes, want %d: %w", errCacheBodyLengthMismatch, bucket, key, bodyLen, meta.ContentLength, bodyErr) + } if bodyErr == nil { - bodyLen := int64(bodyBuf.Len()) if meta.ContentLength > 0 && bodyLen != meta.ContentLength { putBuffer(bodyBuf) s.rememberBodyLengthMismatch(ctx, bucket, key, meta) @@ -406,6 +414,13 @@ func (s *Service) serveFromCache( cw := &lazyCommitWriter{w: w, meta: meta} bodyErr := s.cache.GetBodyStream(ctx, bucket, key, meta.BodyDiscriminator(), cw) if bodyErr != nil && !cw.committed { + if meta.ContentLength > 0 && errors.Is(bodyErr, io.ErrUnexpectedEOF) { + s.rememberBodyLengthMismatch(ctx, bucket, key, meta) + if invalidateOnLengthMismatch != nil { + invalidateOnLengthMismatch() + } + return fmt.Errorf("%w for %s/%s: got 0 bytes, want %d: %w", errCacheBodyLengthMismatch, bucket, key, meta.ContentLength, bodyErr) + } return fmt.Errorf("cache body unavailable: %w", bodyErr) } if !cw.committed {