Skip to content

statistics, schedule: stop republishing metrics for a known-tombstoned store #11166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bufferflies wants to merge 10 commits into
base: master
Choose a base branch
from
Open

statistics, schedule: stop republishing metrics for a known-tombstoned store #11166

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
892b10f
statistics, schedule: stop republishing metrics for a known-tombstone…
bufferflies Aug 20, 2026
d1b7f82
statistics, schedule: close remaining tombstone-cleanup gaps after #1…
bufferflies Aug 20, 2026
d5edd6c
schedulers, statistics, placement: close three more tombstone-metric …
bufferflies Aug 21, 2026
4a04da5
placement: add regression test for toStoreCacheList's stale-snapshot fix
bufferflies Aug 21, 2026
dd84e1b
schedulers, placement, tests: close two more stale-snapshot gaps; fix…
bufferflies Aug 21, 2026
c45456f
schedulers: strengthen TestSummaryPendingInfluenceSkipsRemovedStoreMe…
bufferflies Aug 21, 2026
65b0127
statistics, schedulers, checker: close three more tombstone leaks
bufferflies Aug 21, 2026
89d1b6a
placement, schedulers: fix two correctness bugs found by review
bufferflies Aug 25, 2026
42faef5
placement, schedulers, server: close remaining bury-race gaps found b…
bufferflies Aug 26, 2026
ef71a1d
tests: add regression test for SetLabelStoresLimit skipping tombstone…
bufferflies Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/mcs/scheduling/server/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ func (c *Cluster) SetRuntimeResources(
c.affinityWatcher = affinityWatcher
metaWatcher.SetOnStoreTombstoned(func(storeID uint64) {
c.hotStat.RemoveRollingStoreStats(storeID)
c.ruleManager.RemoveStoreCache(storeID)
DeleteStoreMetrics(strconv.FormatUint(storeID, 10))
})
}
Expand Down
11 changes: 10 additions & 1 deletion pkg/schedule/coordinator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,15 @@ func collectHotMetrics(cluster sche.ClusterInformer, stores []*core.StoreInfo, t
storeAddress := s.GetAddress()
storeID := s.GetID()
storeLabel := strconv.FormatUint(storeID, 10)
// HotPeerCache.gc() only removes a tombstoned store from status.AsLeader/
// AsPeer's source data on its own TTL-throttled schedule, not every tick,
// so a known-tombstoned store here can still have stale hot-peer data.
// Treat it as not hot regardless, so the delete branches below run
// instead of republishing hotSpotStatusGauge every tick until
// HotPeerCache.gc() eventually catches up.
removed := s.IsRemoved()
stat, hasHotLeader := status.AsLeader[storeID]
hasHotLeader = hasHotLeader && !removed
if hasHotLeader {
hotSpotStatusGauge.WithLabelValues(storeAddress, storeLabel, "total_"+kind+"_bytes_as_leader").Set(stat.TotalBytesRate)
hotSpotStatusGauge.WithLabelValues(storeAddress, storeLabel, "total_"+kind+"_keys_as_leader").Set(stat.TotalKeysRate)
Expand All @@ -551,6 +559,7 @@ func collectHotMetrics(cluster sche.ClusterInformer, stores []*core.StoreInfo, t
}

stat, hasHotPeer := status.AsPeer[storeID]
hasHotPeer = hasHotPeer && !removed
Comment thread
bufferflies marked this conversation as resolved.
if hasHotPeer {
hotSpotStatusGauge.WithLabelValues(storeAddress, storeLabel, "total_"+kind+"_bytes_as_peer").Set(stat.TotalBytesRate)
hotSpotStatusGauge.WithLabelValues(storeAddress, storeLabel, "total_"+kind+"_keys_as_peer").Set(stat.TotalKeysRate)
Expand Down Expand Up @@ -578,7 +587,7 @@ func collectHotMetrics(cluster sche.ClusterInformer, stores []*core.StoreInfo, t
// iteration's own s was still live: once a snapshot correctly shows
// IsRemoved(), a tombstoned store sitting in GetStores() for up to 30
// days doesn't cost a scan on every tick.
if !s.IsRemoved() {
if !removed {
if store := cluster.GetStore(storeID); store == nil || store.IsRemoved() {
DeleteStoreMetrics(storeLabel)
}
Expand Down
15 changes: 14 additions & 1 deletion pkg/schedule/placement/region_rule_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ func NewRegionRuleFitCacheManager() *RegionRuleFitCacheManager {
}
}

// RemoveStoreCache removes the store cache with a given store ID.
func (manager *RegionRuleFitCacheManager) RemoveStoreCache(storeID uint64) {
manager.mu.Lock()
defer manager.mu.Unlock()
delete(manager.storeCaches, storeID)
}

// Invalid cache by regionID
func (manager *RegionRuleFitCacheManager) Invalid(regionID uint64) {
manager.mu.Lock()
Expand Down Expand Up @@ -203,7 +210,13 @@ func (manager *RegionRuleFitCacheManager) toStoreCacheList(stores []*core.StoreI
labels: m,
state: s.GetState(),
}
manager.storeCaches[s.GetID()] = sCache
// A removed store's entry is only ever cleared once, when it's
// buried or finally removed; a stale RegionInfo snapshot that
// still lists it as a peer must not re-add it here, or it would
// linger in storeCaches for good since nothing sweeps it again.
if !s.IsRemoved() {
Comment thread
bufferflies marked this conversation as resolved.
Outdated
manager.storeCaches[s.GetID()] = sCache
}
}
c = append(c, sCache)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/schedule/placement/rule_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,11 @@ func (m *RuleManager) InvalidCache(regionID uint64) {
m.cache.Invalid(regionID)
}

// RemoveStoreCache removes the store cache with a given store ID.
func (m *RuleManager) RemoveStoreCache(storeID uint64) {
m.cache.RemoveStoreCache(storeID)
}

// SetPlaceholderRegionFitCache sets a placeholder region fit cache information
// Only used for testing
func (m *RuleManager) SetPlaceholderRegionFitCache(region *core.RegionInfo) {
Expand Down
4 changes: 3 additions & 1 deletion pkg/schedule/schedulers/hot_region.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,10 @@ func newBaseHotScheduler(
// prepareForBalance calculate the summary of pending Influence for each store and prepare the load detail for
// each store, only update read or write load detail
func (s *baseHotScheduler) prepareForBalance(typ resourceType, cluster sche.SchedulerCluster) {
storeInfos := statistics.SummaryStoreInfos(cluster.GetStores())
stores := cluster.GetStores()
storeInfos := statistics.SummaryStoreInfos(stores)
s.summaryPendingInfluence(storeInfos)
s.stHistoryLoads.GC(stores)
Comment thread
bufferflies marked this conversation as resolved.
storesLoads := cluster.GetStoresLoads()
isTraceRegionFlow := cluster.GetSchedulerConfig().IsTraceRegionFlow()

Expand Down
11 changes: 11 additions & 0 deletions pkg/statistics/hot_peer_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,17 @@ func (f *HotPeerCache) gc() {
}
}
for storeID := range removed {
// regionsOfStore[storeID] is the exact set of regions this store is still
// referenced from in storesOfRegion; read it before deleting so the reverse
// index doesn't keep a stale storeID around for regions that are still active.
for regionID := range f.regionsOfStore[storeID] {
if stores, ok := f.storesOfRegion[regionID]; ok {
delete(stores, storeID)
if len(stores) == 0 {
delete(f.storesOfRegion, regionID)
}
}
}
delete(f.peersOfStore, storeID)
delete(f.regionsOfStore, storeID)
delete(f.thresholdsOfStore, storeID)
Expand Down
10 changes: 10 additions & 0 deletions pkg/statistics/store_collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,15 @@ func (s *storeStatistics) observe(store *core.StoreInfo) {

// ObserveHotStat records the hot region metrics for the store.
func ObserveHotStat(store *core.StoreInfo, stats *StoresStats) {
// A store's RollingStoreStats can be recreated after bury by a StoreHeartbeat
// that was already in flight (HandleStoreHeartbeat only rejects a fully
// unknown store, not a tombstoned one). Without this check, that would make
// this function keep republishing storeStatusGauge every collection tick for
// as long as the entry exists, up to 30 days until final removal, instead of
// stopping once the store is known tombstoned like observe() already does.
if store.IsRemoved() {
return
}
// Store flows.
storeAddress := store.GetAddress()
id := strconv.FormatUint(store.GetID(), 10)
Expand Down Expand Up @@ -359,6 +368,7 @@ func Reset() {
storeStatusGauge.Reset()
placementStatusGauge.Reset()
clusterStatusGauge.Reset()
StoreLimitGauge.Reset()
ResetRegionStatsMetrics()
ResetLabelStatsMetrics()
ResetHotCacheStatusMetrics()
Expand Down
21 changes: 21 additions & 0 deletions pkg/statistics/store_load.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,27 @@ func (s *StoreHistoryLoads) Add(storeID uint64, rwTp utils.RWType, kind constant
load.add(pointLoad)
}

// GC removes history load entries for stores that are no longer alive, so a
// store that's gone doesn't keep its entry for the scheduler's entire
// lifetime (there's otherwise no periodic sweep of this cache at all).
func (s *StoreHistoryLoads) GC(stores []*core.StoreInfo) {
alive := make(map[uint64]struct{}, len(stores))
for _, store := range stores {
if !store.IsRemoved() {
alive[store.GetID()] = struct{}{}
}
}
for i := range s.loads {
for j := range s.loads[i] {
for storeID := range s.loads[i][j] {
if _, ok := alive[storeID]; !ok {
delete(s.loads[i][j], storeID)
}
}
}
}
}

// Get returns the store loads from the history, not one time point.
// In another word, the result is [dim][time].
func (s *StoreHistoryLoads) Get(storeID uint64, rwTp utils.RWType, kind constant.ResourceKind) HistoryLoads {
Expand Down
30 changes: 29 additions & 1 deletion server/cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1790,6 +1790,7 @@ func (c *RaftCluster) BuryStoreLocked(storeID uint64, forceBury bool) error {
// clean up the residual information.
c.prevStoreLimit.Delete(storeID)
c.RemoveStoreLimit(storeID)
c.ruleManager.RemoveStoreCache(storeID)
storeIDStr := strconv.FormatUint(storeID, 10)
statistics.ResetStoreStatistics(storeIDStr)
filter.DeleteStoreMetrics(storeIDStr)
Expand Down Expand Up @@ -2182,7 +2183,6 @@ func (c *RaftCluster) RemoveTombStoneRecords() error {
errs.ZapError(err))
return err
}
c.RemoveStoreLimit(store.GetID())
log.Info("delete store succeeded",
zap.Stringer("store", store.GetMeta()))
}
Expand Down Expand Up @@ -2212,6 +2212,13 @@ func (c *RaftCluster) deleteStore(store *core.StoreInfo) error {
// leaving a series this cleanup just deleted with no later event able to
// find and remove it again.
c.DeleteStore(store)
c.ruleManager.RemoveStoreCache(store.GetID())
// The auto-GC path (checkStores' NodeState_Removed branch) only ever calls
// deleteStore, never RemoveTombStoneRecords, so the store-limit config entry
// needs to be cleared here rather than by the manual remove-tombstone caller
// alone -- otherwise a store reclaimed purely by the 30-day auto-GC timer
// never gets this cleanup at all.
c.RemoveStoreLimit(store.GetID())

@rleungx rleungx Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If RemoveStoreLimit exhausts its persistence retries here, deleteStore still returns success after the store metadata and in-memory record are gone. No later GC pass can retry the cleanup, so a restart can reload the stale store-limit entry and republish its metric.

@lhy1024 lhy1024 Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test for the exhausted-persistence path. It must verify durable state, not only the in-memory schedule config:

func TestDeleteStoreDoesNotLoseStoreLimitCleanup(t *testing.T) {
    re := require.New(t)

    // Create and persist a store-limit entry before injecting the failure.
    // Use a tombstone store and the same storage instance for the cluster and reload.
    re.NoError(cluster.SetStoreLimit(storeID, storelimit.AddPeer, 60))

    const persistFail = "github.com/tikv/pd/server/config/persistFail"
    re.NoError(failpoint.Enable(persistFail, "return(true)"))
    err := cluster.deleteStore(tombstone) // all five cleanup attempts fail
    re.NoError(failpoint.Disable(persistFail))

    // The deletion must report the cleanup failure or enqueue a retry.
    re.Error(t, err)

    // After the retry path runs, reload the config from storage and verify
    // that StoreLimit no longer contains storeID.
}

The test should force every in-method retry to fail, then clear the failure and exercise the retry path added by the fix. The final assertion must reload from durable storage. The current implementation returns success and has no later path that retries RemoveStoreLimit, so the stale entry remains after restart.

@bufferflies bufferflies Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed but not adding the requested test: it would demonstrate an accepted gap rather than a behavior change. Tracked in the PR description's Known limitations -- closing it needs either a startup reconciliation between StoresInfo and the persisted StoreLimit config, or making RemoveStoreLimit's failure retryable/observable, both out of scope here given how narrow the trigger is.

@lhy1024 lhy1024 Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this new call adds another full-config writer to the auto-GC path, but it can lose a concurrent SetAllStoresLimit update. Both methods clone and persist the complete schedule config. Persist builds the persistedConfig before calling SaveConfig, and the etcd backend writes it with an unconditional OpPut; there is no shared lock or version check here.

A deterministic interleaving is:

  1. SetAllStoresLimit builds config A, which still contains storeID, and pauses in SaveConfig.
  2. deleteStore calls RemoveStoreLimit, which successfully persists config B without storeID.
  3. The paused setter resumes and successfully overwrites config B with stale config A.

After a reload or restart, the deleted store ID is present in StoreLimit again. The store metrics collector iterates that map and recreates StoreLimitGauge, so the cleanup added here is not durable. This is distinct from the already discussed all-retries-fail case: both writes succeed.

Please serialize all store-limit config writers, or use a CAS/retry/merge mechanism that preserves the deletion. Add a deterministic durable-state regression test, for example:

type blockingConfigStorage struct {
    storage.Storage
    entered chan struct{}
    release chan struct{}
    once    sync.Once
}

func (s *blockingConfigStorage) SaveConfig(cfg any) error {
    s.once.Do(func() {
        close(s.entered)
        <-s.release
    })
    return s.Storage.SaveConfig(cfg)
}

func TestDeleteStoreLimitDoesNotLoseConcurrentAllStoreUpdate(t *testing.T) {
    re := require.New(t)
    _, opt, err := newTestScheduleConfig()
    re.NoError(err)

    backend := storage.NewStorageWithMemoryBackend()
    rc := newTestRaftCluster(context.Background(), mockid.NewIDAllocator(), opt, backend)
    const storeID = uint64(1)
    tombstone := core.NewStoreInfo(&metapb.Store{
        Id:        storeID,
        NodeState: metapb.NodeState_Tombstone,
    })
    rc.PutStore(tombstone)

    // Seed a durable per-store limit before installing the barrier.
    opt.SetStoreLimit(storeID, storelimit.AddPeer, 60)
    re.NoError(backend.SaveStoreMeta(tombstone.GetMeta()))
    re.NoError(opt.Persist(backend))

    blocked := &blockingConfigStorage{
        Storage: backend,
        entered: make(chan struct{}),
        release: make(chan struct{}),
    }
    rc.storage = blocked

    done := make(chan error, 1)
    go func() {
        done <- rc.SetAllStoresLimit(storelimit.AddPeer, 120)
    }()
    <-blocked.entered

    // The removal write succeeds while the batch setter is paused.
    re.NoError(rc.deleteStore(tombstone))
    close(blocked.release)
    re.NoError(<-done)

    _, reloaded, err := newTestScheduleConfig()
    re.NoError(err)
    re.NoError(reloaded.Reload(backend))
    _, ok := reloaded.GetScheduleConfig().StoreLimit[storeID]
    re.False(ok)
}

This test fails on the current head because the final write restores storeID to durable config.

@lhy1024 lhy1024 Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The barrier in the previous example must allow the second SaveConfig call to complete while the first call is paused. A sync.Once implementation would block both calls. Use an atomic first-call flag instead:

type blockingConfigStorage struct {
    storage.Storage
    entered chan struct{}
    release chan struct{}
    first   atomic.Bool
}

func (s *blockingConfigStorage) SaveConfig(cfg any) error {
    if s.first.CompareAndSwap(false, true) {
        close(s.entered)
        <-s.release
    }
    return s.Storage.SaveConfig(cfg)
}

func TestDeleteStoreLimitDoesNotLoseConcurrentAllStoreUpdate(t *testing.T) {
    re := require.New(t)
    _, opt, err := newTestScheduleConfig()
    re.NoError(err)

    backend := storage.NewStorageWithMemoryBackend()
    rc := newTestRaftCluster(context.Background(), mockid.NewIDAllocator(), opt, backend)
    const storeID = uint64(1)
    tombstone := core.NewStoreInfo(&metapb.Store{
        Id:        storeID,
        NodeState: metapb.NodeState_Tombstone,
    })
    rc.PutStore(tombstone)

    opt.SetStoreLimit(storeID, storelimit.AddPeer, 60)
    re.NoError(backend.SaveStoreMeta(tombstone.GetMeta()))
    re.NoError(opt.Persist(backend))

    blocked := &blockingConfigStorage{
        Storage: backend,
        entered: make(chan struct{}),
        release: make(chan struct{}),
    }
    rc.storage = blocked

    done := make(chan error, 1)
    go func() {
        done <- rc.SetAllStoresLimit(storelimit.AddPeer, 120)
    }()
    <-blocked.entered

    // RemoveStoreLimit must successfully write config B while config A is paused.
    re.NoError(rc.deleteStore(tombstone))
    close(blocked.release)
    re.NoError(<-done)

    _, reloaded, err := newTestScheduleConfig()
    re.NoError(err)
    re.NoError(reloaded.Reload(backend))
    _, ok := reloaded.GetScheduleConfig().StoreLimit[storeID]
    re.False(ok)
}

Add sync/atomic to the imports. The final assertion reads durable state and fails on the current head because the paused full-config write restores the deleted store entry.

@bufferflies bufferflies Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, not fixing here: closing this needs a lock or CAS shared across every store-limit-persisting path (SetAllStoresLimit, RemoveStoreLimit, and any other full-ScheduleConfig writer), which is a config-layer-wide change out of scope for this follow-up. Documented in the PR description's Known limitations, including how deleteStore's new RemoveStoreLimit call widens the existing race's trigger surface. Not adding the requested test either, for the same reason as the persistence-retry-exhaustion limitation above: it would demonstrate an accepted gap rather than a behavior change.

@lhy1024 lhy1024 Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remains blocking on the updated head. The fact that full-config writers were already unsynchronized does not make the new deleteStore -> RemoveStoreLimit caller safe: this PR relies on that caller to make final-removal cleanup durable, while the interleaving above lets two successful writes restore the exact removed-store entry with no later lifecycle event to clean it again. Documenting the race therefore leaves the new behavior incomplete. Please serialize the relevant config mutation/persistence paths or use CAS/retry/merge semantics. The deterministic durable-reload test above (with the corrected atomic.Bool barrier) still fails unchanged on ef71a1dcf and should be added with the fix.

storeIDStr := strconv.FormatUint(store.GetID(), 10)
statistics.DeleteClusterStatusMetrics(store)
statistics.ResetStoreStatistics(storeIDStr)
Expand Down Expand Up @@ -2621,6 +2628,18 @@ func (c *RaftCluster) loadExternalTS() {

// SetStoreLimit sets a store limit for a given type and rate.
func (c *RaftCluster) SetStoreLimit(storeID uint64, typ storelimit.Type, ratePerMin float64) error {
// A tombstoned store's config entry is only ever cleared once, at bury time
// (RemoveStoreLimit); nothing sweeps it again afterward. Without this check,
// setting a limit for an already-tombstoned store re-adds it, and
// StoreLimitGauge stays republished for it until final removal.
//
// GetStore returning nil is deliberately NOT treated the same as removed
// here: callers legitimately set a store's limit before the store itself
// is registered (see testCluster.addRegionStore), so nil just means
// "not created yet," not "already gone."

@rleungx rleungx Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil is also the observable state after final tombstone deletion, not only before registration. The HTTP handler checks existence before calling this method, so a concurrent deleteStore can remove the store after that check and this branch will still persist a new limit for an ID that is already gone; no later store event remains to clean it up.

@bufferflies bufferflies Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this is reachable via the HTTP path. Not adding a store == nil check here: it's ambiguous between "not yet registered" and "already fully removed" -- SetStoreLimit is also called for a store before it's registered (see testCluster.addRegionStore, which seeds a limit before PutStore), and adding store == nil to the rejection broke TestDispatch when tried. There's no way to distinguish the two cases from inside SetStoreLimit without additional state tracking, which is disproportionate for a window this narrow: it requires a concurrent deleteStore to land between the HTTP handler's existence check and the actual SetStoreLimit call (separated by request body parsing). Tracking this in the PR description's Known limitations instead.

if store := c.GetStore(storeID); store != nil && store.IsRemoved() {

@rleungx rleungx Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is not synchronized with BuryStoreLocked: the call can pass it, then burial removes the entry, and the subsequent mutation and persistence recreate it. Because tombstone cleanup is one-shot, the stale config and StoreLimitGauge can survive until final removal.

@lhy1024 lhy1024 Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a deterministic unit test for the check-then-act race. A concurrent test without a barrier is not sufficient:

func TestSetStoreLimitCannotRecreateRemovedStore(t *testing.T) {
    re := require.New(t)

    // Pause SetStoreLimit immediately after its IsRemoved check.
    // The test-only barrier must signal that the check has passed.
    done := make(chan error, 1)
    go func() {
        done <- cluster.SetStoreLimit(storeID, storelimit.AddPeer, 60)
    }()
    <-afterRemovedCheck

    // Bury the store while SetStoreLimit is paused, then release it.
    re.NoError(cluster.BuryStore(storeID, true))
    close(releaseSetStoreLimit)

    re.ErrorIs(<-done, errs.ErrStoreRemoved)
    _, ok := cluster.GetOpts().GetScheduleConfig().StoreLimit[storeID]
    re.False(ok)
}

Please implement the barrier with a test-only failpoint or an equivalent injectable seam, and disable it with cleanup. The test must prove that the operation cannot mutate or persist the tombstoned store after burial; the current check is not atomic with the subsequent config mutation.

@bufferflies bufferflies Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, tracked in the PR description's Known limitations (updated to also cover the final-removal case raised separately below). Not adding the requested test or synchronization: reusing storeStateLock here would deadlock, since RemoveStore/UpStore already hold it while calling SetStoreLimit internally, and a dedicated lock or locked/unlocked split is more surgery than this narrow window warrants.

Comment thread
bufferflies marked this conversation as resolved.
Comment thread
bufferflies marked this conversation as resolved.
return errs.ErrStoreRemoved.FastGenByArgs(storeID)
}
old := c.opt.GetScheduleConfig().Clone()
c.opt.SetStoreLimit(storeID, typ, ratePerMin)
if err := c.opt.Persist(c.storage); err != nil {
Expand Down Expand Up @@ -2800,6 +2819,15 @@ func (c *RaftCluster) UnsetServiceIndependent(name string) {
const networkSlowStoreEvictThreshold = 99

func (c *RaftCluster) adjustNetworkSlowStore(storeID uint64) {
// The gRPC handler's own IsRemoved() check isn't atomic with this call, so an
// in-flight StoreHeartbeat that already passed it can still reach here after
// bury. BuryStoreLocked only clears storeTriggerNetworkSlowEvict once, so a
// re-check here (unlike the metric write below, which has no such guard) is
// the only thing that can stop it from staying republished until final
// removal.
if store := c.GetStore(storeID); store == nil || store.IsRemoved() {

@rleungx rleungx Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This recheck is not atomic with BuryStoreLocked: burial can complete after the check and delete the metric, then this in-flight heartbeat can execute TriggerNetworkSlowEvict and increment the series again. With no later heartbeat for the tombstoned store, the recreated state can survive until final removal.

@lhy1024 lhy1024 Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a deterministic unit test for the equivalent race in adjustNetworkSlowStore:

func TestAdjustNetworkSlowStoreCannotRepublishAfterBury(t *testing.T) {
    re := require.New(t)
    defer storeTriggerNetworkSlowEvict.Reset()

    // Seed a score at or above networkSlowStoreEvictThreshold.
    // Pause after the removed-store recheck and before TriggerNetworkSlowEvict.
    done := make(chan struct{})
    go func() {
        cluster.adjustNetworkSlowStore(storeID)
        close(done)
    }()
    <-afterNetworkSlowCheck

    re.NoError(cluster.BuryStore(storeID, true))
    close(releaseNetworkSlowStore)

    <-done
    re.Zero(testutil.ToFloat64(
        storeTriggerNetworkSlowEvict.WithLabelValues(strconv.FormatUint(storeID, 10)),
    ))
}

The barrier must be test-only and deterministic. The current recheck can pass, burial can delete the label, and the in-flight call can then increment the counter again. The assertion must inspect the metric after the in-flight call completes.

@bufferflies bufferflies Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, same non-atomicity as SetStoreLimit above, but bounded: deleteStore's storeTriggerNetworkSlowEvict.DeleteLabelValues call runs on both the manual remove-tombstone and 30-day auto-GC paths, so any recreated series is cleared by final removal at the latest. Not adding synchronization for the same reason as the SetStoreLimit case.

return
}
if c.GetAvgNetworkSlowScore(storeID) >= networkSlowStoreEvictThreshold {
c.TriggerNetworkSlowEvict(storeID)
storeTriggerNetworkSlowEvict.WithLabelValues(strconv.FormatUint(storeID, 10)).Inc()
Expand Down
Loading