cubemaster: fix the data races in localcache

[dwin-gharibi]

Closes #1375.

Motivation

go test -race ./... in CubeMaster reported 9 races, five of them in pkg/base/localcache
production code. The most serious is put() mutating a *util.CacheValue in place while Get()
readers dereference it: Value is a two-word interface{}, so a torn read can pair one value's type
word with another's data word and the caller's type assertion then dereferences a mismatched pointer.

What this changes

All in CubeMaster/pkg/base/localcache/localcache.go:

1. Cache entries are now immutable once published. put() no longer overwrites the live
CacheValue; it builds a replacement and swaps element.Value under the existing mutex:

localCache.Lock()
prev := element.Value.(*util.CacheValue)
next := &util.CacheValue{Key: prev.Key, Value: val, LastAccess: ..., Expired: expired}
element.Value = next
localCache.valueList.MoveToBack(element)
localCache.Unlock()

Get() correspondingly reads the *CacheValue pointer and does its MoveToBack inside one
critical section, then works from that snapshot. Because nothing mutates a published entry any more,
the subsequent field reads need no lock.

This moves Get's MoveToBack from after the refresh decision to before it. The LRU effect is the
same — exactly one MoveToBack per hit, on access — and it removes the duplicated
lock/MoveToBack block that the DemotionExpiredUse path used to need.

2. consecutiveFailNum — the plain store at :201 becomes atomic.StoreInt64, matching the
atomic.AddInt64 at :197 and the atomic.LoadInt64 in errStrategy.

3. curCacheSize — the read in put() becomes atomic.LoadInt64, matching its atomic writes.

4. Destroy() — no longer assigns chCacheExit = nil (which raced with errStrategy's select).
Idempotency now comes from a sync.Once, so repeated Destroy() calls still cannot double-close.

5. ExpiredUse — errStrategy no longer writes the shared *LocalCacheConfig that Get reads.
LocalCache gains an expiredUse atomic.Bool, seeded from config in NewCache and driven by
errStrategy; Get reads it atomically. The yaml-tagged LocalCacheConfig.ExpiredUse field is
unchanged, so config parsing and every existing test that sets it still work.

Also fixed, because it is the last thing standing between this package and a green -race run:
localcache_test.go:48 reassigned the shared ctx in a loop while spawned goroutines read it. The
context is now derived per iteration and passed as a parameter, which also removes an unintended
2700-deep context chain.

No comment changes.

Testing

New: CubeMaster/pkg/base/localcache/localcache_race_test.go

• TestConcurrentGetAndRefreshOnSameKey — 32 readers × 300 iterations on one hot key with a 1 ms
  expiry, so Get and the async-refresh put interleave continuously. It type-asserts every hit,
  which is what would catch a torn interface{}.
• TestDestroyIsIdempotent — repeated Destroy() does not panic.
• TestConcurrentDestroyDoesNotRaceWithBackgroundLoops — four concurrent Destroy() calls.

Red/green verified — with localcache.go reverted to master, the new tests alone report 4 data
races. With the fix:

$ go test -race -count=1 ./pkg/base/localcache/...
ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/localcache       10.739s
ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/localcache/util   2.118s

Module-wide, go test -race ./...:

	master	this branch
data races	9	2
failing tests incl. TestLocalCache, TestNoValue, TestSaveFile	9	6

CI gates checked locally:

• gofmt -l ./pkg/base/localcache — clean (fmt-check).
• GOOS=linux go build ./pkg/base/localcache/ — clean.

Still red after this change (pre-existing, not touched here)

The 2 remaining races and the 6 remaining failures are all pre-existing on master and belong to other
branches:

• pkg/base/queueworker/queue_test.go:65,67,75 — test-side counter race.
• pkg/base/recov/runtime_test.go vs runtime.go:30 — test-side read of an atomically-written
  counter.
• pkg/base/bufferqueue/bufferqueue_test.go:131,138 — same class (this one is not yet tracked; worth
  its own issue).
• TestDestroySandboxMissingSandboxReturnsNotFound, TestHostChangeGoodBodyReturnsSuccessEnvelope,
  TestTemplateLocalityFilterSelect, Test_local_appendNodeByCluster — fail on master too
  (gomonkey cannot patch binaries on darwin; these need the Linux builder).

Risk / rollout

Moderate — this touches the hot read path of a shared cache. The behavioural contract is unchanged:
same LRU semantics, same expiry decisions, same return values. The notable internal change is that a
Get hit now returns a value snapshot taken at lock time rather than whatever the entry happened to
hold mid-refresh, which is the point.

One extra allocation per put() on an existing key (the replacement CacheValue, ~48 bytes). Given
that put already runs a loader call and a JSON-ish payload, this is not a meaningful cost.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

put() now writes element.Value = next while holding localCache's mutex, but saveFile() (unchanged in this PR, called from Destroy()) reads the same *list.Element.Value field — value.Value.(*util.CacheValue) at line 328 — without holding that mutex.

This is a newly-introduced data race. Before this PR, element.Value was write-once (assigned at PushBack), so saveFile's unlocked read was safe; now every refresh of an existing key writes the field. The race is reachable: asyncRefresh() spawns goroutines that are not tracked by waitGroup, and concurrent Gets can still be inside loadAndRefresh → put while Destroy() runs saveFile. The new race tests never overlap Destroy with an in-flight put, so the reported green -race run wouldn't catch it.

Fix suggestion: snapshot the *util.CacheValue under localCache.Lock() in saveFile (e.g. read value.Value into a local while holding the lock, then gob-encode after), so all readers of element.Value are serialized.

[cubesandboxbot]

Review: cubemaster: fix the data races in localcache

AI-generated review — no human approval implied.

Verdict: Approve with minor comments. The core fix is correct, well-scoped, and backed by good red/green verification.

What I verified against the base tree

• Immutable CacheValue entries. The serious race was put() mutating a published *util.CacheValue in place while Get() readers dereferenced the two-word interface{} (torn type/data read). The fix makes every write to list.Element.Value happen under localCache.Lock() (put builds a replacement and swaps the pointer), and Get/saveFile read it under the same lock. Nothing mutates a published entry in place anymore, so the post-lock field reads in Get (itm.Expired, itm.LastAccess, itm.Value) are safe. sharedCalls single-flights same-key loads, and different-key puts serialize on the same mutex for the swap — the accounting is also now consistent (the old unlocked element.Value read could double-subtract under concurrent puts).
• Atomic accounting. curCacheSize and consecutiveFailNum are now accessed only via atomic ops; no plain reads/writes remain on either field.
• expiredUse atomic.Bool migration. All config.ExpiredUse mutation moves out of errStrategy; Get reads the atomic. I grepped the repo — no code outside this package reads config.ExpiredUse at runtime, so keeping the yaml field as config input is safe.
• Destroy via sync.Once. The racy chCacheExit = nil write is gone; the Once preserves idempotency.
• localcache_test.go ctx fix is correct and removes the shared-read race (and the 2700-deep context chain).
• The new race tests are well-designed; the type-assertion loop in TestConcurrentGetAndRefreshOnSameKey is exactly the right probe for a torn interface{}.

Comments

Minor (non-blocking)

1. Dead nil-guard in Destroy (localcache.go). After this change chCacheExit is never set to nil, so if localCache.chCacheExit == nil { return } inside the destroyOnce closure is unreachable for any cache created via NewCache. For a manually-constructed LocalCache (nil channel) it would mark the cache destroyed via Once and permanently skip cleanup. Consider removing the guard.

2. Hot path now takes the cache-wide mutex twice per Get hit (localcache.go). The PR description says Get reads the pointer and MoveToBacks in one critical section, but the diff keeps them as two separate lock acquisitions. The separation is load-bearing — promoting before the refresh decision would make a failed refresh promote the entry, which TestFailingRefreshDoesNotPromoteTheEntry explicitly forbids — so the code is correct; it just deserves a comment, and it doubles the exclusive-lock frequency on the hot read path of a shared cache.

3. TestConcurrentGetAndRefreshOnSameKey's loads > 0 assertion is trivially satisfied — the first Get (cache miss) loads synchronously, so the loader always runs. The type-assert loop is the real detector; the assertion doesn't prove the refresh path was exercised (e.g., asserting loads > 1 or tracking async-refresh invocations would).

Pre-existing, adjacent to the change

4. put can block forever after Destroy. put sends chShrinkCache <- true (size-1 buffer) once the size threshold is crossed; after Destroy, shrinkCache has exited and nothing drains the channel, so a second post-Destroy put blocks indefinitely. The new Destroy tests sidestep this only because their configured sizes stay far below HighCacheSize. Since this PR touches Destroy, worth draining the channel on shutdown (or a non-blocking send). Pre-existing on master; not introduced here.

On the remaining red

The two remaining races and six remaining failures are, as the PR states, pre-existing and outside this package; nothing in the diff regresses them.

[cubesandboxbot]

Behavior change vs. the PR description. The description says "the LRU effect is the same — exactly one MoveToBack per hit, on access", but on the default config (ExpiredUse: false) the old code returned early from the sync-refresh branch — return r, f, err — without MoveToBack, both on refresh success and on refresh failure without demotion. Moving MoveToBack to before the refresh decision means every found hit now promotes the element, including entries whose loader keeps failing with demotion disabled. Those failing entries are now pinned at the back of the list, so shrinkCache's front-based eviction won't reclaim them; they live until the CacheExpiredRemove ticker (default 24 h) instead of drifting toward the eviction front. If this is intended, it's worth stating in the description; if not, the promotion should stay on the non-refresh hit path only.

[cubesandboxbot]

SetupConfig deliberately guards a nil config (if localCacheConfig == nil { return nil }), so localCache.localCacheConfig can be nil here and this new unconditional localCacheConfig.ExpiredUse dereference turns a nil config into a panic at construction time — previously the panic was deferred to the first Get on an expired entry. No current caller passes nil, so this is latent rather than a live bug, but the new code bypasses the nil guard that SetupConfig provides. Consider localCache.expiredUse.Store(localCacheConfig != nil && localCacheConfig.ExpiredUse) or an explicit nil check.

[fslongjin]

LGTM

Verified the races on origin/master with go test -race, and this branch is clean. The immutable CacheValue swap under the mutex is the right fix for the torn interface{} read; the atomic / sync.Once / expiredUse changes are correct.

[cubesandboxbot]

Dead code now: after this PR nothing assigns chCacheExit = nil (that was the race being removed), so this guard can only be true for a LocalCache not created via NewCache. In that case the surrounding destroyOnce.Do marks the cache as destroyed and a later, legitimate Destroy is silently skipped. Consider removing the guard, or making the "not initialized via NewCache" no-op explicit.

[cubesandboxbot]

The PR description says Get reads the *CacheValue pointer and does its MoveToBack inside one critical section, but the MoveToBack at the end of Get is still a separate lock acquisition. The separation is actually load-bearing — promoting the entry before the refresh decision would make a failed refresh promote the entry, which TestFailingRefreshDoesNotPromoteTheEntry explicitly forbids — so the code is correct, just worth a comment. Note it also means every hit now acquires the cache-wide mutex twice (snapshot read + MoveToBack), doubling exclusive-lock contention on the hot read path of this shared cache.