backend: dedupe the write buffer on every path into an existing read bucket

[martin-k-m]

Framing first, so nobody reads more into this than is there: this is hardening
plus test coverage, not a reported bug. The invariant writeback maintains is
genuinely violated on one path, and I could not construct a trigger for it from
etcd's own callers on main. The reachability section below says exactly how
far I got and where the margin is thin. writeback and merge currently have
no tests at all, and that part of the argument stands on its own.

Problem

txWriteBuffer.writeback handles a non-sequential bucket two different ways depending on whether
the read buffer already has that bucket:

func (txw *txWriteBuffer) writeback(txr *txReadBuffer) {
	for k, wb := range txw.buckets {
		rb, ok := txr.buckets[k]
		if !ok {
			delete(txw.buckets, k)
			if seq, ok := txw.bucket2seq[k]; ok && !seq {
				wb.dedupe()                     // deduped
			}
			txr.buckets[k] = wb
			continue
		}
		if seq, ok := txw.bucket2seq[k]; ok && !seq && wb.used > 1 {
			// assume no duplicate keys
			sort.Sort(wb)                       // NOT deduped
		}
		rb.merge(wb)
	}
	...
}

The second branch relies on bucketBuffer.merge to deduplicate, but merge has an early return:

func (bb *bucketBuffer) merge(bbsrc *bucketBuffer) {
	for i := 0; i < bbsrc.used; i++ {
		bb.add(bbsrc.buf[i].key, bbsrc.buf[i].val)
	}
	if bb.used == bbsrc.used {
		return
	}
	if bytes.Compare(bb.buf[(bb.used-bbsrc.used)-1].key, bbsrc.buf[0].key) < 0 {
		return                                  // no overlap, skips dedupe
	}
	bb.dedupe()
}

That early return is correct for its own purpose, which is avoiding a needless dedupe when the
two buffers do not overlap in key range. It is not a substitute for the dedupe the write buffer
still needed on its own. So when the read buffer's largest key for that bucket sorts strictly
below the write buffer's smallest key, a key written twice inside one lock section survives into
the read buffer as two entries.

The consequence is worse than an extra entry. bucketBuffer.Range binary searches for the first
key greater than or equal to the target, so it returns the older of the two values.
dedupe() keeps the newest (sort.Stable then keep-last-of-run); skipping it makes the read
keep the oldest. UnsafeForEach visits both, stale value first, and a ranged UnsafeRange with
a limit above 1 returns both.

There is a second, smaller problem in the same line: sort.Sort is not stable, so with duplicate
keys present the surviving order is not even defined.

Fix

Call dedupe() in both branches. dedupe sorts internally, so the explicit sort.Sort is no
longer needed.

		if seq, ok := txw.bucket2seq[k]; ok && !seq {
			// The bucket is not written sequentially, so the write buffer may
			// hold the same key more than once. merge() only dedupes when the
			// two buffers overlap, so dedupe here as the new-bucket branch
			// above already does. dedupe sorts as well.
			wb.dedupe()
		}
		rb.merge(wb)

The wb.used > 1 guard is dropped because dedupe() already returns immediately when
used <= 1.

Sequential buckets are unaffected. The Key bucket, which holds the MVCC revision keys, is marked
sequential and its keys are strictly increasing by construction.

Honesty about reachability

I could not construct a trigger from etcd itself on main, and I do not want to overstate this.

Triggering requires both:

1. the same key written twice with UnsafePut inside one Lock()...Unlock() section, and
2. the read buffer's existing maximum key for that bucket sorting strictly below the write
   buffer's minimum key.

Every non-test UnsafePut call site under server/ was checked (storage/mvcc/store.go and
storage/schema/{actions,alarm,auth,auth_roles,auth_users,cindex,confstate,lease,membership,version}.go).
Each writes a given key at most once per lock section, so condition 1 does not hold today.

The near miss is worth stating because it shows how thin the margin is. schema/cindex.go writes
consistent_index and term into the Meta bucket on every apply, via the
txPostLockInsideApplyHook, and repeated applies inside one batch interval do reach the
rb.merge(wb) branch. That path is safe only because "term" sorts after "consistent_index",
so the overlap check fires and dedupe() runs. It is safe by key ordering, not by design.

So: a latent invariant violation, one caller away from being live, whose failure mode is a silent
stale read rather than a crash. Reviewers may reasonably prefer this framed as hardening plus
test coverage rather than as a bug fix. The test coverage argument stands on its own:
tx_buffer_test.go currently exercises dedupe and CopyUsed in isolation and has no test for
writeback or merge at all.

Test

TestWritebackDedupesExistingBucket drives writeback directly with a write buffer holding
zzz=old then zzz=new, against a read buffer already holding one key. The two subcases differ
only in that seeded key: "aaa" sorts below zzz so merge takes the no-overlap fast path,
"zzzz" sorts above so it does not. Both must yield a single entry with value new.

Having both subcases is the point. It isolates the trigger to merge's fast path rather than
just asserting an outcome.

Test evidence

Run on Linux, golang:1.26 container.

Without the fix:

=== RUN   TestWritebackDedupesExistingBucket/no_overlap_between_buffers
        	Error:      	Not equal:
        	            	expected: []string{"new"}
        	            	actual  : []string{"old", "new"}
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,3 @@
        	            	-([]string) (len=1) {
        	            	+([]string) (len=2) {
        	            	+ (string) (len=3) "old",
        	            	  (string) (len=3) "new"
        	Test:       	TestWritebackDedupesExistingBucket/no_overlap_between_buffers
        	Messages:   	writeback must keep the newest value
--- FAIL: TestWritebackDedupesExistingBucket (0.00s)
    --- FAIL: TestWritebackDedupesExistingBucket/no_overlap_between_buffers (0.00s)
    --- PASS: TestWritebackDedupesExistingBucket/overlapping_buffers (0.00s)
FAIL
FAIL	go.etcd.io/etcd/server/v3/storage/backend	0.007s

Note the overlapping subcase passes without the fix. That is the control.

With the fix:

=== RUN   TestWritebackDedupesExistingBucket
--- PASS: TestWritebackDedupesExistingBucket (0.00s)
    --- PASS: TestWritebackDedupesExistingBucket/no_overlap_between_buffers (0.00s)
    --- PASS: TestWritebackDedupesExistingBucket/overlapping_buffers (0.00s)
PASS
ok  	go.etcd.io/etcd/server/v3/storage/backend	0.008s

Package under the race detector:

ok  	go.etcd.io/etcd/server/v3/storage/backend	2.994s

Full server module unit suite on Linux: no failures.

Lint:

### LINT ###
0 issues.
'lint' PASSED and completed at Sat Aug 15 14:50:22 UTC 2026
SUCCESS

Performance note to expect in review

dedupe() sorts with sort.Stable where the old path used sort.Sort. Stable sort is slower and
allocates. This runs once per writeback per non-sequential bucket, and non-sequential buckets
are the small metadata ones, not the Key bucket. I do not expect it to be measurable, but if a
reviewer pushes back, the alternative is to keep sort.Stable only when a duplicate is actually
possible, which would need bucket2seq to carry more information than it does now.

Checklist

• Test fails without the fix and passes with it, both states shown above
• Control subcase demonstrates the trigger is merge's fast path
• make verify-lint clean
• Full server module unit tests pass
• DCO sign-off on the commit
• No issue filed, on purpose: see the reachability section. Filing a bug
  report for something with no demonstrated live trigger would overstate it.
• Framed as hardening plus coverage, which is the weaker of the two readings
• Rebased on current main

---

This PR was written in part with the assistance of generative AI. Every change was reviewed, built and tested before submitting, and no AI co-author or assisted-by trailers are used, per https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#ai-guidance

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: martin-k-m
Once this PR has been reviewed and has the lgtm label, please assign fuweid 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 @martin-k-m. 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.