tikv / tikv/pd

mcs: expected-primary transient marker (#10952) has unguarded races that let /primary/transfer silently no-op or be bypassed

Open
#11,122 0 comments 0 reactions 0 assignees View on GitHub
type/bug
Dominant language
Go
Stars
1.2k
Forks
783
Avg merge
5d 21h
Merged PRs (30d)
36

Description

## Bug Report

### What did you do?

Reviewed `pkg/mcs/utils/expected_primary.go` as introduced by aa5a98865 (`mcs, tso: replace expected-primary keepalive with transient marker (#10952)`), and compared it against a downstream fork's port of the same commit, where a longer code review round (multiple reviewers, several follow-up rounds) surfaced and fixed four correctness gaps in this exact mechanism. I re-checked all four against current `master` (`afa43111d`) — no commit has touched `pkg/mcs/utils/expected_primary.go` since aa5a98865, so all four are still present.

The mechanism: `{service}/primary/transfer` writes a TTL-bound `expected_primary` marker naming the transfer target, then resigns the current primary; the election loop only lets the named target campaign, and the winner deletes the marker. All four issues are races around that marker's write/read/guard lifecycle.

### What did you expect to see?

A `{service}/primary/transfer` call should either durably move the primary to the intended target, or fail — never return success while silently not moving anything, and never let an unrelated member win a campaign the marker was supposed to constrain.

### What did you see instead?

**1. `markExpectedPrimaryFlag` writes the marker with no leadership guard** (`pkg/mcs/utils/expected_primary.go`, ~line 60)

```go
func markExpectedPrimaryFlag(client *clientv3.Client, msParam *keypath.MsParam, primary *primaryData, leaseID clientv3.LeaseID) error {
...
resp, err := kv.NewSlowLogTxn(client).
Then(clientv3.OpPut(path, primary.raw, clientv3.WithLease(leaseID))).
Commit()
...
}
```

No `If()` clause at all. `TransferPrimary` checks `p.IsServing()` once at the top, then does `discovery.GetMSMembers` and a `client.Grant` (two network round trips) before this write. If the caller loses leadership anywhere in that window, the stale call still publishes the marker and returns success — but a serving primary only reconciles the marker once, right after winning its own campaign, so the real (different) primary never looks at it again. The transfer silently no-ops while reporting success.

**2. `DeleteExpectedPrimaryFlag` can't tell "marker gone" from "marker overwritten by a newer transfer"** (~line 93)

```go
func DeleteExpectedPrimaryFlag(client *clientv3.Client, msParam *keypath.MsParam, expectedValue string) {
...
resp, err := kv.NewSlowLogTxn(client).
If(clientv3.Compare(clientv3.Value(path), "=", expectedValue)).
Then(clientv3.OpGet(path), clientv3.OpDelete(path)).
Commit()
...
if !resp.Succeeded {
log.Info("skip deleting expected primary flag, it has been changed or already gone", ...)
return
}
...
}
```

`resp.Succeeded == false` is logged and swallowed identically whether the marker is simply absent (nothing to do) or a *newer* transfer already rewrote it to point at a different member while this campaigner was winning. In the second case, this member is about to start serving anyway (its own campaign already committed), and the newer transfer's real target is never notified or promoted — that second `/primary/transfer` call returns success but achieves nothing beyond a marker that lingers until its own TTL.

**3. `ExpectedPrimaryCmp` returns no guard at all for the "no transfer observed" case** (~line 137)

```go
func ExpectedPrimaryCmp(msParam *keypath.MsParam, expectedValue string) *clientv3.Cmp {
if expectedValue == "" {
return nil
}
cmp := clientv3.Compare(clientv3.Value(keypath.ExpectedPrimaryPath(msParam)), "=", expectedValue)
return &cmp
}
```

A campaigner that read an empty marker appends `nil` — i.e. no comparison — to its campaign transaction. If a transfer installs a marker naming a different target and releases the leader key after that read but before this campaigner's commit, the campaign transaction has nothing stopping it from winning anyway, bypassing the transfer affinity guard the marker exists to enforce.

**4. Guarding on the leader key's `Value` (rather than `CreateRevision`) doesn't fence a specific election term**

This is the fix for #1 that still isn't quite enough on its own: `Participant.MemberValue()` is fixed for the participant's whole lifetime, so `clientv3.Compare(clientv3.Value(leaderKey), "=", memberValue)` also matches after the *same* member loses its lease and wins a fresh campaign with that identical `MemberValue()` — a different election term than the one `IsServing()` originally checked, but indistinguishable by value. `Leadership.Campaign` requires `CreateRevision(leaderKey) == 0` to win, so `CreateRevision` changes on every fresh campaign even when the winner and its `MemberValue()` are unchanged; guarding on `CreateRevision` (captured right after the `IsServing()` check, before discovery and the lease grant — not right before the write) is what actually fences the marker write to the specific serving instance the caller validated.

---

Fixes and tests for all four exist already (found/fixed during downstream review), not yet submitted upstream. Happy to open a PR with the fix + regression tests for each if that's useful — wanted to file the analysis first so it's visible for triage independent of any specific patch.

### What version of PD are you using (`pd-server -V`)?

`master` @ `afa43111d` (current HEAD as of this report). Bug originates in aa5a98865 / #10952.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/mcs/utils/expected_primary.go and trace /primary/transfer through markExpectedPrimaryFlag, DeleteExpectedPrimaryFlag, ExpectedPrimaryCmp, and the campaign path. Compare the downstream fixes and regression tests mentioned in the report. Done means leadership loss cannot publish a stale marker, marker changes are distinguished, and campaigns cannot bypass transfer affinity.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.