ordering: Txn.Commit reissues a mutable transaction after a revision violation

[logical-misha]

Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via security@etcd.io.
• This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• I have read the etcd bug reporting guidelines.
• Existing open issues and the etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

ordering.NewKV reissues a transaction whose successful response has a header
revision older than the wrapper's previous observed revision whenever
orderViolationFunc returns nil. The loop does not distinguish read-only
transactions from transactions whose selected branch may write.

The first KV.Do call has already returned a transaction response with a nil
error, so the wrapper cannot assume that its mutation did not execute. Sending
the same transaction body again can duplicate effects, increment versions
again, emit extra watch events, or execute a different branch if intervening
state changed.

What did you expect to happen?

The ordering wrapper should not automatically resubmit a transaction after an
execution that may have mutated state. It could restrict automatic retries to
transactions known to be read-only or surface the ordering violation through a
non-retrying path. The response/error policy is separate; the required safety
property is that one Commit call does not execute the mutable body twice.

How can we reproduce it (as minimally and precisely as possible)?

Add this unit test to client/v3/ordering/kv_test.go and run:

cd client/v3
go test ./ordering -run '^TestTxnOrderingDoesNotRetryMutableTxn$' -count=1

type mockTxnReplayKV struct {
	clientv3.KV
	responses []clientv3.OpResponse
	index     int
}

func (kv *mockTxnReplayKV) Do(ctx context.Context, op clientv3.Op) (clientv3.OpResponse, error) {
	resp := kv.responses[kv.index]
	kv.index++
	return resp, nil
}

func TestTxnOrderingDoesNotRetryMutableTxn(t *testing.T) {
	staleTxnResponse := &clientv3.TxnResponse{
		Header:    &pb.ResponseHeader{Revision: 9},
		Succeeded: true,
	}
	freshTxnResponse := &clientv3.TxnResponse{
		Header:    &pb.ResponseHeader{Revision: 10},
		Succeeded: true,
	}
	mKV := &mockTxnReplayKV{
		KV: clientv3.NewKVFromKVClient(nil, nil),
		responses: []clientv3.OpResponse{
			staleTxnResponse.OpResponse(),
			freshTxnResponse.OpResponse(),
		},
	}
	var violationCalled bool
	kv := &kvOrdering{
		KV: mKV,
		orderViolationFunc: func(op clientv3.Op, resp clientv3.OpResponse, prevRev int64) error {
			violationCalled = true
			return nil
		},
		prevRev: 10,
		revMu:   sync.RWMutex{},
	}

	// The response/error policy after detecting the violation is not asserted.
	// This test only requires that the successful mutable operation is not sent
	// a second time.
	_, _ = kv.Txn(context.TODO()).Then(clientv3.OpPut("mockKey", "mockValue")).Commit()
	if !violationCalled {
		t.Fatal("expected stale mutable txn response to be reported")
	}
	if mKV.index != 1 {
		t.Fatalf("expected one mutable txn execution, got %d", mKV.index)
	}
}

Anything else we need to know?

The current txnOrdering.Commit
implementation
builds one OpTxn and loops over kv.KV.Do until
the returned transaction revision reaches the previous observed revision. The
provided NewOrderViolationSwitchEndpointClosure returns nil for its initial
violations, so the resubmission path is not limited to custom callbacks.

The original motivation in #7623 and the package
documentation
focus on
stale serializable reads. This unit reproduction establishes the wrapper's
behavior once a successful low-revision mutable response is observed; it does
not claim that a healthy, directly connected etcd cluster normally produces
such a response for a committed write. The prerequisite could instead arise
from a custom KV implementation, middleware, or an endpoint/cluster
misconfiguration. NewKV accepts the generic clientv3.KV interface and does
not document mutable transactions as unsupported; if they are outside the
intended contract, rejecting or documenting them would also remove the unsafe
ambiguity.

Etcd version (please run commands below)

Confirmed on:

$ git rev-parse HEAD
12caed621b38312cc1084113415bf135c59e6457

$ go version
go version go1.26.5 linux/amd64

also reproduced at v3.6.13 (b0f9ef190952e6e66a778513097a02ee41220727)

Etcd configuration (command line flags or environment variables)

No server configuration is needed; this is a client-side unit test.

Etcd debug information

Not applicable.

Relevant log output

--- FAIL: TestTxnOrderingDoesNotRetryMutableTxn
    kv_test.go: expected one mutable txn execution, got 2
FAIL go.etcd.io/etcd/client/v3/ordering