fix: fail closed on unreadable legacy tombstones

README.md

@@ -104,6 +104,7 @@ When configuring S3 clients, ensure path-style addressing is enabled. See [docs/
 - Set `block_caching_enabled: false` to cache whole objects instead — a range request then triggers a background fetch of the full object (if within threshold)
 - In tiered mode, a PUT routed to the local tier checks a concrete SigV4 `X-Amz-Content-Sha256` digest after admission and any local precondition pass and the full body passes its declared-length and any Content-MD5 checks; conditional writes against upstream-tier markers, `UNSIGNED-PAYLOAD`, and aws-chunked uploads keep their existing paths
 - PUT/DELETE operations invalidate the cache entry
+- Legacy ordered metadata populates are refused when the tombstone cannot be read or is malformed, rather than assuming no invalidation. The detached metadata warm remains best-effort, so a successful S3 write response is unchanged.
 - If a whole-object cache write returns an error during a successful GET, the upstream response is unchanged and the cache-populate slot becomes available after relay cleanup, so a later eligible GET can retry. A store call that never returns has no completion guarantee.
 
 See [docs/cache-control.md](docs/cache-control.md) for detailed cache control and revalidation documentation. For tuning and sizing block caching in production, see [docs/deploy.md](docs/deploy.md#block-aligned-caching).

cache/coordinator.go

@@ -40,6 +40,8 @@ import (
 	"github.com/rs/zerolog/log"
 	cacheclient "github.com/tigrisdata/ocache/client"
 	"github.com/tigrisdata/tag/metrics"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 // metaCoordinator is the ordering seam for the mutable meta key.
@@ -290,8 +292,15 @@ func (c *legacyCoordinator) putMeta(ctx context.Context, bucket, key, metaKey st
 	if expected != VersionAny {
 		// Tombstone gate right before the visibility-granting meta write: an
 		// invalidation at or after the caller's decision-time stamp blocks the
-		// commit, so a populate cannot resurrect deleted metadata.
-		if ts := c.tombstoneTimestamp(ctx, bucket, key); ts >= int64(expected) {
+		// commit, so a populate cannot resurrect deleted metadata. If the marker
+		// cannot be read or decoded, its state is unknown, not absent: refuse the
+		// visibility write and let the caller handle this as a cache-write error.
+		ts, err := c.tombstoneTimestamp(ctx, bucket, key)
+		if err != nil {
+			metrics.RecordCacheOperation("meta_put", "error")
+			return false, err
+		}
+		if ts >= int64(expected) {
 			metrics.RecordCacheOperation("meta_put", "precondition_lost")
 			log.Debug().Str("bucket", bucket).Str("key", key).
 				Int64("tombstone_ts", ts).Uint64("write_start", expected).
@@ -360,12 +369,25 @@ func (c *legacyCoordinator) writeTombstone(ctx context.Context, bucket, key stri
 	return c.client.Put(ctx, MakeTombstoneKey(bucket, key), data, c.tombstoneTTL)
 }
 
-func (c *legacyCoordinator) tombstoneTimestamp(ctx context.Context, bucket, key string) int64 {
+func (c *legacyCoordinator) tombstoneTimestamp(ctx context.Context, bucket, key string) (int64, error) {
 	data, err := c.client.Get(ctx, MakeTombstoneKey(bucket, key))
-	if err != nil || len(data) != 8 {
-		return 0
+	if err != nil {
+		// The production embedded client represents absence as nil, nil; the
+		// gRPC and in-memory clients return a typed NotFound status. Other errors
+		// (including routing errors whose text contains "not found") leave the
+		// marker state unknown and must not authorize a metadata write.
+		if data == nil && status.Code(err) == codes.NotFound {
+			return 0, nil
+		}
+		return 0, fmt.Errorf("read tombstone for %s/%s: %w", bucket, key, err)
+	}
+	if data == nil {
+		return 0, nil
+	}
+	if len(data) != 8 {
+		return 0, fmt.Errorf("malformed tombstone for %s/%s: got %d bytes, want 8", bucket, key, len(data))
 	}
-	return int64(binary.BigEndian.Uint64(data))
+	return int64(binary.BigEndian.Uint64(data)), nil
 }
 
 // deleteMetaIfVersion is a CAS-strength identity delete: legacy coordination

cache/coordinator_tombstone_test.go

@@ -0,0 +1,160 @@
+package cache
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus/testutil"
+	cacheclient "github.com/tigrisdata/ocache/client"
+	ocachecoordinator "github.com/tigrisdata/ocache/coordinator"
+	"github.com/tigrisdata/tag/config"
+	"github.com/tigrisdata/tag/metrics"
+)
+
+type tombstoneGetOverrideClient struct {
+	cacheclient.CacheClient
+	markerKey string
+	data      []byte
+	err       error
+	override  bool
+}
+
+func (c *tombstoneGetOverrideClient) Get(ctx context.Context, key string) ([]byte, error) {
+	if c.override && key == c.markerKey {
+		return c.data, c.err
+	}
+	return c.CacheClient.Get(ctx, key)
+}
+
+func TestLegacyCoordinator_TombstoneReadErrorAndMalformedMarkerRefuseMetaWrite(t *testing.T) {
+	const bucket, key = "b", "k"
+	markerKey := MakeTombstoneKey(bucket, key)
+	backendErr := errors.New("cache read unavailable")
+	routingErr := ocachecoordinator.NewNodeNotFoundError("missing-node", markerKey)
+	tests := []struct {
+		name          string
+		data          []byte
+		err           error
+		override      bool
+		wantCause     error
+		wantErrorText string
+	}{
+		{
+			name:      "backend read failure",
+			err:       backendErr,
+			override:  true,
+			wantCause: backendErr,
+		},
+		{
+			name:      "routing error containing not found",
+			err:       routingErr,
+			override:  true,
+			wantCause: ocachecoordinator.ErrNodeNotFound,
+		},
+		{
+			name:          "empty marker is malformed",
+			data:          make([]byte, 0),
+			override:      true,
+			wantErrorText: "malformed tombstone",
+		},
+		{
+			name:          "short marker is malformed",
+			data:          []byte{1, 2, 3},
+			override:      true,
+			wantErrorText: "malformed tombstone",
+		},
+		{
+			name:          "long marker is malformed",
+			data:          make([]byte, 9),
+			override:      true,
+			wantErrorText: "malformed tombstone",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := config.NewDefault()
+			cfg.Cache.SetLegacyCoordination(true)
+			client := &tombstoneGetOverrideClient{
+				CacheClient: cacheclient.NewMemoryCache(),
+				markerKey:   markerKey,
+				data:        tc.data,
+				err:         tc.err,
+				override:    tc.override,
+			}
+			c := NewCacheWithClient(client, &cfg.Cache)
+			ctx := context.Background()
+
+			old, expected, found, err := c.GetMetaWithVersion(ctx, bucket, key)
+			if err != nil || found || old != nil || expected == 0 {
+				t.Fatalf("decision token = (meta=%+v expected=%d found=%v err=%v), want absent metadata and a nonzero token", old, expected, found, err)
+			}
+
+			meta := &CachedObjectMeta{Bucket: bucket, Key: key, ETag: `"stale"`, StatusCode: 200}
+			metaPutErrorsBefore := testutil.ToFloat64(metrics.CacheOperations.WithLabelValues("meta_put", "error"))
+			wrote, writeErr := c.PutMetaIfVersion(ctx, bucket, key, meta, 60, expected)
+			if got := testutil.ToFloat64(metrics.CacheOperations.WithLabelValues("meta_put", "error")); got != metaPutErrorsBefore+1 {
+				t.Errorf("meta_put/error count = %v, want one additional error outcome", got)
+			}
+			if wrote {
+				t.Fatal("metadata was published despite an unreadable or malformed tombstone")
+			}
+			if writeErr == nil {
+				t.Fatal("PutMetaIfVersion returned nil error for an unreadable or malformed tombstone")
+			}
+			if tc.wantCause != nil && !errors.Is(writeErr, tc.wantCause) {
+				t.Fatalf("PutMetaIfVersion error %v does not preserve cause %v", writeErr, tc.wantCause)
+			}
+			if tc.wantErrorText != "" && !strings.Contains(writeErr.Error(), tc.wantErrorText) {
+				t.Fatalf("PutMetaIfVersion error = %v, want text %q", writeErr, tc.wantErrorText)
+			}
+			if _, found, err := c.GetMeta(ctx, bucket, key); err != nil || found {
+				t.Fatalf("metadata after refused write: found=%v err=%v, want absent", found, err)
+			}
+		})
+	}
+}
+
+func TestLegacyCoordinator_TombstoneAbsenceStillAllowsMetaWarm(t *testing.T) {
+	const bucket, key = "b", "k"
+	for _, tc := range []struct {
+		name     string
+		override bool
+	}{
+		{name: "typed not found from cache client"},
+		{name: "embedded nil result", override: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := config.NewDefault()
+			cfg.Cache.SetLegacyCoordination(true)
+			client := &tombstoneGetOverrideClient{
+				CacheClient: cacheclient.NewMemoryCache(),
+				markerKey:   MakeTombstoneKey(bucket, key),
+				override:    tc.override,
+			}
+			c := NewCacheWithClient(client, &cfg.Cache)
+			ctx := context.Background()
+
+			_, expected, found, err := c.GetMetaWithVersion(ctx, bucket, key)
+			if err != nil || found || expected == 0 {
+				t.Fatalf("decision token = (expected=%d found=%v err=%v), want an absent key and nonzero token", expected, found, err)
+			}
+
+			meta := &CachedObjectMeta{Bucket: bucket, Key: key, ETag: `"current"`, StatusCode: 200}
+			metaPutErrorsBefore := testutil.ToFloat64(metrics.CacheOperations.WithLabelValues("meta_put", "error"))
+			wrote, err := c.PutMetaIfVersion(ctx, bucket, key, meta, 60, expected)
+			if got := testutil.ToFloat64(metrics.CacheOperations.WithLabelValues("meta_put", "error")); got != metaPutErrorsBefore {
+				t.Errorf("meta_put/error count = %v, want no additional error outcome for a genuinely absent tombstone", got)
+			}
+			if err != nil || !wrote {
+				t.Fatalf("PutMetaIfVersion on absent tombstone = (%v, %v), want (true, nil)", wrote, err)
+			}
+			got, found, err := c.GetMeta(ctx, bucket, key)
+			if err != nil || !found || got == nil || got.ETag != meta.ETag {
+				t.Fatalf("metadata after warm = (%+v, %v, %v), want current ETag", got, found, err)
+			}
+		})
+	}
+}

docs/configuration.md

@@ -189,11 +189,12 @@ cache:
   block_caching_enabled: true
 
   # Meta invalidation/populate coordination. true (default) = legacy tombstone mechanism,
-  # compatible with any cluster mix — including nodes upgrading directly from <=v1.20. false =
-  # fenced-CAS coordination (stronger ordering; requires every node in the cluster to be
-  # CAS-capable BEFORE flipping, then a brisk rolling restart). Standalone deployments may flip
-  # immediately. Override with TAG_CACHE_LEGACY_COORDINATION (fail-safe: only an explicit
-  # "false"/"0" selects CAS).
+  # compatible with any cluster mix — including nodes upgrading directly from <=v1.20. A missing
+  # tombstone permits a warm; a failed read or malformed marker refuses an ordered metadata write
+  # instead of assuming no invalidation. false = fenced-CAS coordination (stronger ordering;
+  # requires every node in the cluster to be CAS-capable BEFORE flipping, then a brisk rolling
+  # restart). Standalone deployments may flip immediately. Override with
+  # TAG_CACHE_LEGACY_COORDINATION (fail-safe: only an explicit "false"/"0" selects CAS).
   legacy_coordination: true
 
   # Block granularity AND the whole-vs-block boundary for every populate path: an object
@@ -404,7 +405,7 @@ Controls the embedded cache behavior. TAG uses an embedded OCache instance with
 | `ttl`                   | duration | `24h`            | Default TTL for cached objects                                                      |
 | `size_threshold`        | int64    | `1073741824`     | Max object size to cache (bytes)                                                    |
 | `block_caching_enabled` | bool     | `true`           | Enable block-aligned caching for large objects (RFC 0001)                           |
-| `legacy_coordination`   | bool     | `true`           | Meta coordination mechanism: `true` = legacy tombstones (safe for any cluster mix, incl. direct ≤v1.20 upgrades); `false` = fenced CAS (all nodes must be CAS-capable before flipping) |
+| `legacy_coordination`   | bool     | `true`           | Meta coordination mechanism: `true` = legacy tombstones (safe for any cluster mix, incl. direct ≤v1.20 upgrades; unreadable or malformed markers refuse ordered metadata writes); `false` = fenced CAS (all nodes must be CAS-capable before flipping) |
 | `block_size`            | int64    | `1048576`        | Block granularity **and** the read-side whole-vs-block boundary (must stay below ocache's 64 MB compaction threshold) |
 | `parquet_optimization`  | bool     | `false`          | Prefetch a parquet object's metadata blocks on a tail read (only when the metadata spans more than the tail block) |
 | `disk_path`             | string   | `/var/cache/tag` | Path to cache data directory                                                        |

docs/metrics.md

@@ -112,10 +112,16 @@ Total number of cache misses.
 
 Total number of cache operations.
 
-| Label       | Description                               |
-| ----------- | ----------------------------------------- |
-| `operation` | Operation type: `get`, `put`, `delete`    |
-| `result`    | Result: `hit`, `miss`, `success`, `error` |
+| Label       | Description                                                              |
+| ----------- | ------------------------------------------------------------------------ |
+| `operation` | Operation type: `get`, `put`, `meta_put`, `delete`                       |
+| `result`    | Result: `hit`, `miss`, `success`, `error`, `precondition_lost`           |
+
+`meta_put` denotes metadata publication outcomes. `precondition_lost` means an
+ordered metadata write was refused: in legacy coordination, no decision token was
+supplied (`expected == 0`) or a newer tombstone was observed; in CAS coordination,
+a `PutIfVersion` version mismatch occurred. With legacy coordination, an unreadable
+or malformed tombstone is recorded as `error` and the write is refused.
 
 **Example queries:**