tikv / tikv/pd

resource_group: cross-term Add/Modify/Delete can leave a stale cache entry after publishResourceGroupMutation's confirmed-write guard

Open
#11,105 4 comments 0 reactions 0 assignees View on GitHub
needs-type
Dominant language
Go
Stars
1.2k
Forks
783
Avg merge
4d 15h
Merged PRs (30d)
33

Description

### Context

In #10873, `publishResourceGroupMutation` (pkg/mcs/resourcemanager/server/manager.go) was changed to skip applying a mutation's cache effect when the current (possibly new-term) keyspace manager already has confirmed data for the target group, to avoid an old-term mutation clobbering a newer confirmed write after a leadership change. See the "Known gap" comment on that function for the full reasoning.

### The gap

The guard assumes the confirming write's storage effect is genuinely the latest one. That holds for Add/Modify, which both persist to storage *before* they can be parked mid-flight (see failpoints `addResourceGroupBeforePublish` / `modifyResourceGroupBeforePublish`, both placed after the storage write). It does not hold for Delete, whose failpoint `deleteResourceGroupBeforeStorage` parks it *before* its storage phase.

Interleaving:
1. An old-term `DeleteResourceGroup(g)` is parked before its storage removal (leadership changes here).
2. A new-term `AddResourceGroup(g)` persists and publishes, becoming "confirmed".
3. The parked old-term Delete resumes and finally executes its storage removal, deleting the group added in step 2 — Delete is genuinely the last storage writer.
4. The old Delete's publish step then runs `publishResourceGroupMutation`, sees the current manager already has confirmed data for `g` (from step 2), and skips applying the Delete's cache effect.

Result: storage has no group `g`, but the serving cache still returns it, indefinitely — nothing re-syncs it afterward, since it reads as "confirmed" to every other path too.

This mirrors, in the opposite direction, a gap the old unconditional-apply code had (a delayed Delete publish could instead wipe a newer confirmed Add).

### Why not fixed inline

Closing this fully needs a storage-side revision/CAS check to determine which write actually landed last (the same class of gap tracked for `initDefaultResourceGroup`'s `stillCurrent` check — see the `defaultGroupMu` discussion in the same PR). That's a larger change touching the storage interface and both backends, out of scope for #10873.

### Suggested next steps

- At minimum, add a regression test pinning this interleaving (e.g. `TestAsyncLoadResourceGroupsCrossTermDeleteVsCompetingAdd`) so it's caught rather than silently tolerated.
- Ideally, introduce a conditional (CAS) write at the storage layer, similar to `initOrGetClusterID`'s `clientv3.Compare(clientv3.CreateRevision(key), "=", 0)` pattern in `pkg/storage/endpoint/cluster_id.go`, so publish order can be determined authoritatively instead of via in-memory identity/epoch heuristics.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/mcs/resourcemanager/server/manager.go, read publishResourceGroupMutation and the failpoints around Delete and Add. Run or extend the suggested TestAsyncLoadResourceGroupsCrossTermDeleteVsCompetingAdd to reproduce the interleaving; done means the stale cache case is pinned down, while a full fix would require investigating the storage interface and the CAS pattern in pkg/storage/endpoint/cluster_id.go.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, databases, distributed-systems, testing
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.