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

[cristifalcas]

This was done by Claude.

Fixes #22376.

A key deleted and re-created within a single main revision loses its value when the store is compacted at exactly that revision. The next range over the key hits the range failed to find revision pair fatal at mvcc/kvstore_txn.go:128, the process exits, and the restarted member rebuilds an empty index from a backend that no longer holds the rows — serving an empty keyspace while reporting itself healthy.

This is a regression from #18274, which changed the generation-selection condition in keyIndex.doCompact from tomb > atRev to tomb >= atRev. Verified by building the unit reproducer at bbdc94181^ (passes) and bbdc94181 (fails).

The defect

For a key that existed before the rewrite, one transaction that deletes and re-creates it produces:

gen0: [ ..., 4.0 put, 7.2 tombstone ]   <- the DeleteRange
gen1: [ 7.5 put ]                        <- the Put, same main revision

doCompact(7) breaks on gen0 because its tombstone's main revision equals atRev, so gen1 is never walked and 7.5 never reaches available. scheduleCompaction then deletes that row from the backend, while compact() leaves gen1 in the index still referencing it.

The fix

Advance past a generation whose tombstone sits on atRev only when the next generation was created in the same main revision — i.e. the key was re-created and the tombstone is superseded. keyIndex.since() already encodes this rule, hiding superseded revisions of a main revision from watchers, so the key is alive at the end of atRev and its put must survive.

A tombstone that is the key's last word at atRev is still kept, so #18274's behaviour is unchanged.

Compaction hash values do not change

keyIndex.keep() feeds newKVHasher and is required to stay consistent across versions. I enumerated every put/tombstone sequence of length 4 over a small main/sub revision space and compared keep() and compact()'s available for every atRev, against both the pre-#18274 algorithm and current main (5285 cases):

comparison	keep() diffs	compact() available diffs
fixed vs pre-#18274 (≤ 3.5.15 / ≤ 3.4.33)	0	624
fixed vs current main	126	131

• keep() is byte-identical to pre-*: keep tombstone if revision == compactAtRev #18274 in every case, so HashByRev is restored to exactly the value older versions produce. The 126 differences against current main are precisely the affected shape, where main has already deleted live data.
• The 624 compact() differences against pre-*: keep tombstone if revision == compactAtRev #18274 are *: keep tombstone if revision == compactAtRev #18274's own intentional change (keeping the tombstone at the compaction revision), already compensated for in hash.go; this PR does not add to them.
• Every revision whose membership in available changes has Main == atRev (asserted in the same sweep). The compaction hasher only gates membership for revisions with Main <= prevCompactRev < compactMainRev, so it never consults the entries that changed.

Empirically, compaction hashes over workloads that do not contain the affected shape — including #18274's own tombstone-at-compaction-revision shape and repeated delete/re-create across revisions — are bit-identical before and after this change (22 (workload, revision) pairs compared).

There are 5 shapes in the sweep where the result differs from both prior versions, all the same one: delete, re-create and delete again inside one revision. There the fix keeps the revision's final tombstone; current main keeps a superseded earlier one and pre-#18274 kept neither. That is what #18274 intended, and it is not a data-loss case.

Tests

• TestKeyIndexCompactAndKeepOnRecreatedRev — the generation selection, asserting compact() and keep() separately, with the *: keep tombstone if revision == compactAtRev #18274 tombstone case as an explicit regression guard.
• TestStoreCompactRecreatedKeyInSameRevision — store level over a real backend: DeleteRange + Put in one write txn, compact at that revision, range the keys, then re-open the store over the same backend to prove the index still rebuilds.
• TestReproduce22376 — e2e, via the public client API.

All three fail on main and pass with the fix. make verify-lint, go test -race ./storage/..., tests/common (integration), and the integration compaction/corruption/hash/watch suites are green.

Backports

The regressing commit is on every supported branch (release-3.5 from v3.5.16, release-3.6, release-3.7; release-3.4 is EOL). I confirmed the defect reproduces on origin/release-3.4 and origin/release-3.5 with an equivalent unit reproducer. Happy to open backport PRs for 3.5, 3.6 and 3.7 with their CHANGELOG entries once this is reviewed.

Note on reachability

checkIntervals in server/etcdserver/api/v3rpc/key.go is meant to reject a txn that deletes and puts the same key, but it builds the delete's interval as [key, range_end). For range_end == "\x00" — clientv3.WithFromKey(), the idiomatic "delete to the end of the keyspace" — that interval is empty and intersects nothing, so the overlapping puts are accepted. Details and a measured table of which forms are caught are in #22376. Whether to tighten that check is a separate question; the mvcc layer needs to be correct either way, since the apply path does not validate and the same entry applies unchecked on followers and on WAL replay.

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cristifalcas
Once this PR has been reviewed and has the lgtm label, please assign siyuanfoundation for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @cristifalcas. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.