From edf94357c334d1c68854f626128d240c0a4f3562 Mon Sep 17 00:00:00 2001 From: Cristian Falcas Date: Thu, 3 Sep 2026 16:00:57 +0100 Subject: [PATCH] mvcc: keep the live value of a key re-created in the compacted revision A key that is deleted and re-created within a single main revision is alive at the end of that revision: the later sub revision supersedes the tombstone, as keyIndex.since() already encodes when it hides superseded revisions from watchers. keyIndex.doCompact stopped at the generation ending with the tombstone whenever its main revision equalled atRev, so the following generation -- holding the re-creation at a higher sub revision of the same main revision -- was never walked. The live revision was left out of the `available` set and scheduleCompaction deleted its row from the backend, while compact() kept that generation in the index still pointing at it. The next range over the key hit the "range failed to find revision pair" fatal, and the restarted member rebuilt an empty index from a backend that no longer held the rows, serving an empty keyspace while reporting itself healthy. Advance past a generation whose tombstone sits on atRev only when the next generation was created in that same main revision. A tombstone that is the key's last word at atRev is still kept, preserving #18274. keep() is unaffected: over an exhaustive enumeration of key index shapes its output is identical to the pre-#18274 behaviour, so HashByRev values are unchanged. Every revision whose membership in `available` changes has Main == atRev, which the compaction hasher never gates on, so compaction hashes are unchanged as well. Fixes #22376 Signed-off-by: Cristian Falcas --- CHANGELOG/CHANGELOG-3.8.md | 1 + server/storage/mvcc/key_index.go | 17 +++- server/storage/mvcc/key_index_test.go | 123 ++++++++++++++++++++++++++ server/storage/mvcc/kvstore_test.go | 55 ++++++++++++ tests/e2e/reproduce_22376_test.go | 81 +++++++++++++++++ 5 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/reproduce_22376_test.go diff --git a/CHANGELOG/CHANGELOG-3.8.md b/CHANGELOG/CHANGELOG-3.8.md index 801a96bcd6f..974f5a8c649 100644 --- a/CHANGELOG/CHANGELOG-3.8.md +++ b/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 diff --git a/server/storage/mvcc/key_index.go b/server/storage/mvcc/key_index.go index 14035197417..c9aa6455d61 100644 --- a/server/storage/mvcc/key_index.go +++ b/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. diff --git a/server/storage/mvcc/key_index_test.go b/server/storage/mvcc/key_index_test.go index 643ba11da99..fa469a161c1 100644 --- a/server/storage/mvcc/key_index_test.go +++ b/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 { diff --git a/server/storage/mvcc/kvstore_test.go b/server/storage/mvcc/kvstore_test.go index 8c2255f2e23..ddac4567aee 100644 --- a/server/storage/mvcc/kvstore_test.go +++ b/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) diff --git a/tests/e2e/reproduce_22376_test.go b/tests/e2e/reproduce_22376_test.go new file mode 100644 index 00000000000..8de33852e4c --- /dev/null +++ b/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) +}