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

Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via [email protected].
• 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