A stalled PD leader's step-down is untested, and while it blocks the member still answers and acts as the leader
- Dominant language
- Go
- Stars
- 1.2k
- Forks
- 783
- Avg merge
- 5d 21h
- Merged PRs (30d)
- 36
Description
## What happened
A PD member's data volume stopped completing writes and never recovered. Its embedded etcd stopped making raft progress but continued to report itself as the etcd leader, so PD never took the path that makes a member step down. The lease holding the key that names the PD leader continued to be renewed by a healthy peer, so the key did not expire. The other members read that key, remained in `watch`, and never campaigned. In production the state persisted for 48 minutes and ended only when the key was deleted by hand.
Two independent conditions were required.
**C1 - the ownership check reads a value the stall itself froze.** PD determines whether it is still the etcd leader by comparing `GetEtcdLeader()` against its own ID. That reads `m.etcd.Server.Lead()`, whose only writer is `updateLead` in the etcd `Ready` loop. The loop persists each `Ready` synchronously before consuming the next, so once it blocked in the WAL write it never reached the `Ready` carrying the new term. Every ownership check in PD reads this single value.
**C2 - the lease can be renewed by any member of the cluster.** In `v8.5.3`, `CreateEtcdClient` unconditionally installed a health checker that rewrites the client's endpoints to the healthy members, so the stalled member's keepalives were served by a peer and continued to succeed. This is intended etcd behaviour rather than a defect: a lease belongs to the cluster, not to the member that first accepted it.
C1 is the defect. C2 determined the severity: without it the lease would have expired within one TTL and another member would have taken over, which is the difference between a leader that does not step down and a cluster in which no member can take over.
## Reported occurrences
| | #7780 (2024) | #10746 (QA, `v8.5.4`) | #10671 (prod, `v8.5.3`) |
| --- | --- | --- | --- |
| duration | ~5 min, until the `Ready` loop caught up | the whole injection window | never resolved - 48 minutes, ended by hand |
| impact | leaders non-colocated, suboptimal | same, plus followers could not campaign | no PD could take over |
All three are the same defect. The severity varies only with how long the `Ready` loop remains blocked. The WAL/Ready blocking location is a high-confidence source-code inference supported by stalled persistence and divergent leader-view metrics; the incident has no goroutine dump establishing the exact stack. The production evidence is recorded in [a comment on #10671](https://github.com/tikv/pd/issues/10671#issuecomment-5174037257) and is not repeated here.
## Three layers of fix
Each layer is a stage of the failure, reachable only once the layer above it holds:
| Layer | Failure state | Where on the step-down path | Status |
| --- | --- | --- | --- |
| **1. The term never ends** | the lease is renewed through a peer, so the key never expires and no member can take over | / | **fixed** by #9986 / #10007; #11110 adds what was missing around it |
| **2. The term ends, the identity remains** | leadership is reported without consulting `IsServing()`, and the stores that clear - or publish - the identity sit on the wrong side of calls that can block | **first half**, `Resign()`, and the tail of step-up | **implemented in open PR #11147** |
| **3. The identity is cleared, the work remains** | the `RaftCluster` background jobs keep running; five of them write to etcd with no leader guard, and the coordinator can still hand operators to TiKV | **second half**, `stopRaftCluster()` | **implemented in open PR #11177** - the signal to the jobs; the wait stays unbounded, see below |
### Layer 1 - the term never ends
**Already fixed by #9986 / #10007**, which pin the election client to the local member: a stalled member can no longer renew through a peer, so the key disappears within one TTL. First release carrying it is `v8.5.5`, and all three reports are from earlier releases, so **what an affected cluster needs is this change on its own release branch**; the first-layer mechanism and its version boundary are separate from the identity and cancellation changes below.
#9986 landed for TSO availability, though - it closes #9981, "Improve the high availability of the tso and **election**" - so the leadership-safety consequence was never recorded and no test asserts it. That consequence is that a renewal must be answered by this member's own etcd, which serves it only after a linearizable check against live raft status, so a stalled member cannot renew even while its cached view still says it is the leader. The same change also moved TSO timestamp storage onto that client, so a later change made for TSO availability is a plausible way to bring C2 straight back. #11110 supplies the documentation and the tests.
### Layer 2 - the term ends, the identity remains
A member that has given up its term continues to report itself as the leader until `unsetLeader` runs. Two paths report leadership without consulting `IsServing()`: `GetMembers` reads `GetLeader` directly, and the v1 redirector handles a request locally when the cached leader name is its own; the microservice primary loops have the same exposure through `GetServingUrls`. The store that clears the identity sat behind a lease revoke and the log calls around it, which have no upper bound on a stalled volume. #11147 orders the in-memory stores ahead of both. The same window exists in the other direction and in the TSO allocator's reset: the ready-to-serve log sat between the identity publication and the watchdog loop, and `resetTimestamp` logged ahead of the resign. #11147 puts the identity operation on the safe side of the log in both directions - cleared before anything that can block on the way down, published after it on the way up.
### Layer 3 - the identity is cleared, the work remains
The `RaftCluster` and its background jobs are created on the server context, not on the term (`server/server.go:550`; `InitCluster` derives `c.ctx` from `c.serverCtx`). What ends them is `RaftCluster.Stop()`, from a `stopRaftCluster()` defer that runs after the lease teardown in `Resign()`. Until that returns the jobs keep running: milliseconds in an ordinary step-down, the whole stall in this one.
`RaftCluster.Start` launches 11 top-level jobs (`server/cluster/cluster.go:466`), the scheduling controller 3 more in classic mode, plus 4 `ratelimit.ConcurrentRunner`s. Nine of the 14 jobs touch only in-process state, only read, write behind a guard, or push to a downstream that eventually drops them. **Five write to etcd with no leader guard:**
| Job | Write |
| --- | --- |
| `runMinResolvedTSJob` | `SaveMinResolvedTS` |
| `runStoreConfigSync` | `SaveConfig` - the whole persisted config |
| `runReplicationMode` | `SaveReplicationStatus` - DR state |
| `runNodeStateCheckJob` | `SaveStoreMeta` / `DeleteStoreMeta` - the cluster topology |
| `runCoordinator` | `SaveSchedulerConfig` from `InitSchedulers` and from the schedulers' own persist paths |
Those writes commit. `etcdKVBase.Save` is an unconditional transaction with no `If()` on the leader key (`pkg/storage/kv/etcd_kv.go:95`); the storage is built on `s.client` (`server/server.go:534`), the health-checked etcd client, so a member whose own etcd has stalled has them routed to a healthy peer and into the live quorum; and PD's existing guarded writes (`LeaderTxn`, `RunInTxn` with a leader compare, one hand-written `clientv3.Compare`) are on other paths. `runCoordinator` has a second channel that does not go through etcd: `s.hbStreams` is created on the server context (`server/server.go:582`) and drops a bound stream only when a `Send` fails, so operators keep reaching TiKV until each store's next store heartbeat is rejected (default 10s). TiKV's epoch check rejects most of them, not all.
**#11177** splits `Stop()` into its two halves - take the cluster out of service and cancel `c.ctx`; stop the runners and wait - and calls the first from `resetLeader` before `Member.Resign()`, as `RaftCluster.Cancel()`. The intended contract is to send the cluster cancellation before the blocking resign path. Cancellation delivery across concurrent Start and Cancel still needs targeted validation on #11177; see its unresolved review discussion. `Cancel()` takes no lock, because `runServiceCheckJob` holds `c.RLock()` across the very `SaveTimestamp` that stalls; the waiting half of `Stop()`, the TSO allocator reset and the GC state manager callback stay where they are. The PR spells out those decisions.
What remains after it: a write in flight at the instant of cancellation can still commit, and a goroutine already inside a blocked system call is not interrupted - cancellation is cooperative, so work already entered or selected can still continue. The fences that would close those are follow-ups, split out when started:
- [ ] Leader-key fencing on `etcdKVBase` writes, the way `SaveTimestamp` does it. Must be parameterised per storage instance: the TSO and resource-manager microservices share the type and hold their own primaries.
- [ ] A term check on the `hbstream` send path, after measuring how long the window is and what fraction of operators sent inside it pass TiKV's epoch check.
- [ ] A term guard on `SyncRegions`, answering with a `Header.Error` response rather than a gRPC status error, so the router microservice re-resolves instead of reconnecting to the same address.
- [ ] Descriptor-driven tests that every `pdpb.PD` method is either guarded or role-agnostic, and that each guarded one is rejected on a non-serving member.
## What is deliberately not done
1. **The stale signal itself.** `Lead()` stays exactly as stale and `GetEtcdLeader()` returns it unchanged. That is #7780, and it sits underneath all three layers rather than being one of them. A future consumer that turns the stale value back into a safety decision would need its own fix.
2. **The waiting half of layer 3.** Cleanup has no upper bound if file logging blocks under the stall, and whether it does is genuinely undecided. Bounding it means giving cleanup a timed contract - the blocking points are six bare `wg.Wait()` calls covering eleven background jobs whose exit paths log before returning - and dropping a goroutine that holds a lock or is mid-write is not a decision to make in passing. After cancellation, jobs may remain in blocked operations, and in-flight effects are not fenced by cancellation alone. Completion of cleanup and rejoining as a follower remain separate conditions; a persistently blocked member may require a restart.
3. **The hand-off window is not fenced.** No PD term in `pdpb.RegionHeartbeatResponse.Header`, and TiKV validates only `RegionEpoch`. But heartbeat-driven dispatch is cut off the moment the lease goes invalid, and the same unfenced window exists in every ordinary leader transfer ; this investigation has not measured its duration or the fraction of commands that remain valid. The etcd and `hbstream` fences listed under layer 3 are what would change this; they are follow-ups, not part of any of the three PRs.
4. **Known boundary, not a gap.** The lease proves *this member's etcd can make raft progress*, not that the PD is healthy. A deployment whose log or region-storage volume stalls while the etcd data volume stays healthy keeps renewing normally.
## Current status and next steps
Checked on 2026-09-08:
- #11110 merged on September 2. It documents and tests the existing election-client pinning and changes no production behavior.
- #11147 is open at `27cb1a669ca07884aa0513f93c36c3790acee6fe`. lhy1024 approved this head on September 3. It still needs the remaining LGTM and resolution of the failed `pull-unit-test-next-gen-3` job.
- #11177 is open and ready for review at `397e99d271016d5a1923143097002c9c33cb4e56`, based on #11147. Its Start/Cancel interleaving needs targeted validation; it also needs maintainer review and permission for the gated CI jobs.
Proceed in order: resolve #11147's merge blockers and merge it; then restack #11177, resolve its remaining correctness and CI/review issues, validate the final head, and merge it.
#11147 closes #10671 and #10746. Those reports describe the earlier first-layer failure; neither contains production evidence of the later residual-write paths. #11177 closes this issue for the scoped cancellation change. The fences and waiting-boundary questions above remain explicitly separate follow-ups rather than claims of completed protection.
#7780 remains open: none of these PRs updates the stale `Lead()` signal itself.
Related: #10671, #10746, #7780, #11110, #11147, #11177, #9986, [etcd#13527](https://github.com/etcd-io/etcd/issues/13527), [etcd#16822](https://github.com/etcd-io/etcd/pull/16822)
### Latest verification
On `27cb1a669`, the failed subtest was run locally from `tests/integrations` with:
```sh
go test ./tso -tags=nextgen,without_dashboard -run TestLegacyTSOConsistencySuite/TestRequestTSOConcurrently -count=3
```
All three runs passed (73.298s), with failpoints enabled for the test and disabled afterwards. No code was changed. This does not establish that the CI failure is flaky; the failed CI job still needs a fresh result and further investigation if it recurs.
Contributor guide
Research direction
Start with the layer-2 and layer-3 entry points named in the issue: resetLeader, Member.Resign, RaftCluster.Cancel/Stop, and server/server.go. Read #11147 and #11177, then run the targeted validation for Start/Cancel interleaving and the affected unit tests. Done requires resolving the open PR blockers and validating cancellation without expanding into the explicitly deferred fencing and cleanup work.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100