etcd-io / etcd-io/etcd

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

Open
#22,086 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
52.3k
Forks
10.5k
Avg merge
2d 21h
Merged PRs (30d)
43

Description

### Bug report criteria

- [x] This bug report is not security related, security issues should be disclosed privately via security@etcd.io.
- [x] This is not a support request or question, support requests or questions should be raised in the etcd [discussion forums](https://github.com/etcd-io/etcd/discussions).
- [x] I have read the etcd [bug reporting guidelines](https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/reporting_bugs.md).
- [x] Existing open issues and the etcd [frequently asked questions](https://etcd.io/docs/latest/faq/) 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:

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

```go
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](https://github.com/etcd-io/etcd/blob/12caed621b38312cc1084113415bf135c59e6457/client/v3/ordering/kv.go#L135-L157)
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](https://github.com/etcd-io/etcd/blob/12caed621b38312cc1084113415bf135c59e6457/client/v3/ordering/doc.go#L15-L24)
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:

```console
$ 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

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

Contributor guide

Open the contributing guide

Research direction

Start in client/v3/ordering/kv.go at txnOrdering.Commit and review client/v3/ordering/kv_test.go. Run the provided TestTxnOrderingDoesNotRetryMutableTxn reproduction, then trace how successful low-revision transaction responses enter the retry loop. Done means the mutable transaction body is submitted only once while the ordering violation is still reported, with the package tests passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.