mvcc: keep the live value of a key re-created in the compacted revision

CHANGELOG/CHANGELOG-3.8.md

@@ -12,6 +12,7 @@ Previous change logs can be found at [CHANGELOG-3.7](https://github.com/etcd-io/
   - [Remove flag `--max-snapshots` and `--v2-deprecation`](https://github.com/etcd-io/etcd/pull/22306)
   - [Cleanup the legacy v2 snapshot files on bootstrap](https://github.com/etcd-io/etcd/pull/22336)
   - [Cleanup the legacy v2 snapshot source code and cleanup orphaned defragmentation files on bootstrap](https://github.com/etcd-io/etcd/pull/22341)
+- Fix [compaction deleting the live value of a key that was deleted and re-created in the compacted revision](https://github.com/etcd-io/etcd/pull/22377), which caused a `range failed to find revision pair` fatal and an empty keyspace after the restart.
 
 ### Dependencies
 

server/storage/mvcc/key_index.go

@@ -269,7 +269,16 @@ func (ki *keyIndex) doCompact(atRev int64, available map[Revision]struct{}) (gen
 	genIdx, g := 0, &ki.generations[0]
 	// find first generation includes atRev or created after atRev
 	for genIdx < len(ki.generations)-1 {
-		if tomb := g.revs[len(g.revs)-1].Main; tomb >= atRev {
+		tomb := g.revs[len(g.revs)-1].Main
+		if tomb > atRev {
+			break
+		}
+		// A tombstone at exactly atRev is kept, so this generation is the one
+		// to compact into -- unless the key was re-created within the same main
+		// revision, in which case the higher sub revision supersedes the
+		// tombstone and the key is alive at the end of atRev. Its live revision
+		// lives in the next generation and must survive the compaction.
+		if tomb == atRev && !ki.generations[genIdx+1].isRecreatedAt(atRev) {
 			break
 		}
 		genIdx++
@@ -351,6 +360,12 @@ type generation struct {
 
 func (g *generation) isEmpty() bool { return g == nil || len(g.revs) == 0 }
 
+// isRecreatedAt reports whether this generation was created by a put in main
+// revision rev, i.e. the key was deleted and re-created within that revision.
+func (g *generation) isRecreatedAt(rev int64) bool {
+	return !g.isEmpty() && g.revs[0].Main == rev
+}
+
 // walk walks through the revisions in the generation in descending order.
 // It passes the revision to the given function.
 // walk returns until: 1. it finishes walking all pairs 2. the function returns false.

server/storage/mvcc/key_index_test.go

@@ -608,6 +608,129 @@ func TestKeyIndexCompactAndKeep(t *testing.T) {
 	}
 }
 
+// TestKeyIndexCompactAndKeepOnRecreatedRev covers keys that are deleted and
+// re-created within a single main revision (a DeleteRange followed by a Put in
+// one transaction) and are then compacted at exactly that revision. The later
+// sub revision supersedes the tombstone, so the key is alive at the end of the
+// revision and its live revision must survive the compaction.
+func TestKeyIndexCompactAndKeepOnRecreatedRev(t *testing.T) {
+	tests := []struct {
+		name    string
+		build   func(lg *zap.Logger) *keyIndex
+		compact int64
+
+		wki      *keyIndex
+		wcompact map[Revision]struct{}
+		wkeep    map[Revision]struct{}
+	}{
+		{
+			name: "deleted and re-created in the compacted revision",
+			build: func(lg *zap.Logger) *keyIndex {
+				ki := &keyIndex{key: []byte("foo")}
+				ki.put(lg, 3, 89)
+				require.NoError(t, ki.tombstone(lg, 4, 0))
+				ki.put(lg, 4, 145)
+				return ki
+			},
+			compact: 4,
+			wki: &keyIndex{
+				key:      []byte("foo"),
+				modified: Revision{Main: 4, Sub: 145},
+				generations: []generation{
+					{created: Revision{Main: 4, Sub: 145}, ver: 1, revs: []Revision{{Main: 4, Sub: 145}}},
+				},
+			},
+			wcompact: map[Revision]struct{}{{Main: 4, Sub: 145}: {}},
+			wkeep:    map[Revision]struct{}{{Main: 4, Sub: 145}: {}},
+		},
+		{
+			name: "deleted, re-created and deleted again in the compacted revision",
+			build: func(lg *zap.Logger) *keyIndex {
+				ki := &keyIndex{key: []byte("foo")}
+				ki.put(lg, 1, 0)
+				require.NoError(t, ki.tombstone(lg, 2, 0))
+				ki.put(lg, 2, 1)
+				require.NoError(t, ki.tombstone(lg, 2, 2))
+				return ki
+			},
+			compact: 2,
+			wki: &keyIndex{
+				key:      []byte("foo"),
+				modified: Revision{Main: 2, Sub: 2},
+				generations: []generation{
+					{created: Revision{Main: 2, Sub: 1}, ver: 2, revs: []Revision{{Main: 2, Sub: 2}}},
+					{},
+				},
+			},
+			// the last tombstone of the revision is kept, the superseded one is not
+			wcompact: map[Revision]struct{}{{Main: 2, Sub: 2}: {}},
+			wkeep:    map[Revision]struct{}{},
+		},
+		{
+			// regression guard for #18274
+			name: "tombstone at the compacted revision is kept",
+			build: func(lg *zap.Logger) *keyIndex {
+				ki := &keyIndex{key: []byte("foo")}
+				ki.put(lg, 2, 0)
+				ki.put(lg, 3, 0)
+				require.NoError(t, ki.tombstone(lg, 4, 0))
+				return ki
+			},
+			compact: 4,
+			wki: &keyIndex{
+				key:      []byte("foo"),
+				modified: Revision{Main: 4},
+				generations: []generation{
+					{created: Revision{Main: 2}, ver: 3, revs: []Revision{{Main: 4}}},
+					{},
+				},
+			},
+			wcompact: map[Revision]struct{}{{Main: 4}: {}},
+			wkeep:    map[Revision]struct{}{},
+		},
+		{
+			name: "re-created in a later revision keeps the tombstone",
+			build: func(lg *zap.Logger) *keyIndex {
+				ki := &keyIndex{key: []byte("foo")}
+				ki.put(lg, 1, 0)
+				require.NoError(t, ki.tombstone(lg, 2, 0))
+				ki.put(lg, 3, 0)
+				return ki
+			},
+			compact: 2,
+			wki: &keyIndex{
+				key:      []byte("foo"),
+				modified: Revision{Main: 3},
+				generations: []generation{
+					{created: Revision{Main: 1}, ver: 2, revs: []Revision{{Main: 2}}},
+					{created: Revision{Main: 3}, ver: 1, revs: []Revision{{Main: 3}}},
+				},
+			},
+			wcompact: map[Revision]struct{}{{Main: 2}: {}},
+			wkeep:    map[Revision]struct{}{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			lg := zaptest.NewLogger(t)
+
+			ki := tt.build(lg)
+			kiClone := cloneKeyIndex(ki)
+			keep := make(map[Revision]struct{})
+			ki.keep(tt.compact, keep)
+			require.Equalf(t, kiClone, ki, "keep must not modify the keyIndex for %q", tt.name)
+			require.Equal(t, tt.wkeep, keep)
+
+			ki = tt.build(lg)
+			am := make(map[Revision]struct{})
+			ki.compact(lg, tt.compact, am)
+			require.Equal(t, tt.wcompact, am)
+			require.Equal(t, tt.wki, ki)
+		})
+	}
+}
+
 func cloneKeyIndex(ki *keyIndex) *keyIndex {
 	generations := make([]generation, len(ki.generations))
 	for i, gen := range ki.generations {

server/storage/mvcc/kvstore_test.go

@@ -30,7 +30,9 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
 	"go.uber.org/zap/zaptest"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/testing/protocmp"
@@ -371,6 +373,59 @@ func TestStoreCompact(t *testing.T) {
 	}
 }
 
+// TestStoreCompactRecreatedKeyInSameRevision writes a key, then deletes and
+// re-creates it in a single transaction, and compacts at that revision. The
+// live value must survive the compaction: before the fix its backend row was
+// deleted while the index kept pointing at it, so the next range over the key
+// hit the "range failed to find revision pair" fatal, and a restart rebuilt an
+// empty keyspace from the backend.
+func TestStoreCompactRecreatedKeyInSameRevision(t *testing.T) {
+	// turn the fatal in rangeKeys into a recoverable panic
+	lg := zaptest.NewLogger(t, zaptest.WrapOptions(zap.WithFatalHook(zapcore.WriteThenPanic)))
+	b, _ := betesting.NewDefaultTmpBackend(t)
+	s := NewStore(lg, b, &lease.FakeLessor{}, StoreConfig{})
+	defer func() {
+		s.Close()
+		b.Close()
+	}()
+
+	s.Put([]byte("foo"), []byte("bar"), lease.NoLease)
+	s.Put([]byte("foo2"), []byte("bar2"), lease.NoLease)
+
+	// one transaction rewriting the whole keyspace: every key gets a tombstone
+	// and its re-creation in the same main revision, at different sub revisions
+	tw := s.Write(traceutil.TODO())
+	tw.DeleteRange([]byte("foo"), []byte("fop"))
+	tw.Put([]byte("foo"), []byte("bar-new"), lease.NoLease)
+	tw.Put([]byte("foo2"), []byte("bar2-new"), lease.NoLease)
+	tw.End()
+
+	rev := s.Rev()
+	ch, err := s.Compact(traceutil.TODO(), rev)
+	require.NoError(t, err)
+	<-ch
+
+	assertRange := func(t *testing.T, s KV) {
+		t.Helper()
+		tr := s.Read(ConcurrentReadTxMode, traceutil.TODO())
+		defer tr.End()
+		r, err := tr.Range(t.Context(), []byte("foo"), []byte("fop"), RangeOptions{})
+		require.NoError(t, err)
+		require.Len(t, r.KVs, 2)
+		require.Equal(t, []byte("bar-new"), r.KVs[0].Value)
+		require.Equal(t, []byte("bar2-new"), r.KVs[1].Value)
+	}
+
+	assertRange(t, s)
+
+	// the backend must still be able to rebuild the index, i.e. a restart does
+	// not come back with an empty keyspace
+	s.Close()
+	s = NewStore(lg, b, &lease.FakeLessor{}, StoreConfig{})
+	require.Equal(t, rev, s.Rev())
+	assertRange(t, s)
+}
+
 func TestStoreRestore(t *testing.T) {
 	lg := zaptest.NewLogger(t)
 	s := newFakeStore(lg)

tests/e2e/reproduce_22376_test.go

@@ -0,0 +1,81 @@
+// Copyright 2026 The etcd Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package e2e
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	clientv3 "go.etcd.io/etcd/client/v3"
+	"go.etcd.io/etcd/tests/v3/framework/e2e"
+)
+
+// TestReproduce22376 reproduces the issue: https://github.com/etcd-io/etcd/issues/22376
+//
+// A key that is deleted and re-created within a single main revision must keep
+// its value when the store is compacted at exactly that revision. Before the
+// fix the live value was deleted from the backend while the index still
+// referenced it, so the next range hit the "range failed to find revision pair"
+// fatal and the restarted member served an empty keyspace.
+func TestReproduce22376(t *testing.T) {
+	e2e.BeforeTest(t)
+	// bound the context: if the member dies on the fatal below, the client
+	// would otherwise retry until the test binary times out
+	ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second)
+	defer cancel()
+
+	clus, err := e2e.NewEtcdProcessCluster(ctx, t, e2e.WithClusterSize(1))
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, clus.Stop()) })
+
+	cli := newClient(t, clus.EndpointsGRPC(), e2e.ClientConfig{})
+
+	keys := []string{"cfg/a", "cfg/b", "cfg/c"}
+	for _, k := range keys {
+		_, err = cli.Put(ctx, k, "v1")
+		require.NoError(t, err)
+	}
+
+	// Rewrite the whole keyspace in one transaction, so that every key gets a
+	// tombstone and its re-creation in the same main revision.
+	ops := []clientv3.Op{clientv3.OpDelete("\x00", clientv3.WithFromKey())}
+	for _, k := range keys {
+		ops = append(ops, clientv3.OpPut(k, "v2"))
+	}
+	txnResp, err := cli.Txn(ctx).Then(ops...).Commit()
+	require.NoError(t, err)
+
+	_, err = cli.Compact(ctx, txnResp.Header.Revision, clientv3.WithCompactPhysical())
+	require.NoError(t, err)
+
+	assertKeys := func(t *testing.T) {
+		t.Helper()
+		resp, gerr := cli.Get(ctx, "cfg/", clientv3.WithPrefix())
+		require.NoError(t, gerr)
+		require.Len(t, resp.Kvs, len(keys))
+		for _, kv := range resp.Kvs {
+			require.Equal(t, []byte("v2"), kv.Value)
+		}
+	}
+
+	assertKeys(t)
+
+	// the backend must still hold enough to rebuild the index on restart
+	require.NoError(t, clus.Procs[0].Restart(ctx))
+	assertKeys(t)
+}