tests: fix the data races in queueworker, recov and bufferqueue

[dwin-gharibi]

Closes #1478.

Motivation

go test -race fails in three packages because tests read state written by a goroutine without
synchronization. The production code is correct in all three cases; the tests are wrong.

These are part of what stands between the repo and a green -race gate.

What this changes

queueworker/queue_test.go — TestQueueBlock (CubeMaster and Cubelet copies)

got becomes an atomic.Bool, and the test now waits on a done channel closed by the goroutine
instead of sleeping for a fixed second:

• removes the race on got
• removes the unconditional 1s sleep, so the test finishes as soon as BPop returns
• fails with a clear message if BPop never returns, instead of reporting a confusing assertion failure

recov/runtime_test.go — TestGoWithRetryWithCrash (CubeMaster and Cubelet copies)

int(panicTime) becomes int(atomic.LoadInt32(&panicTime)), matching the atomic.AddInt32 that
production HandleCrash uses to write it.

bufferqueue/bufferqueue_test.go — TestQueueCheckTimestampPriority (CubeMaster only)

The watcher goroutine is given a watcherDone channel and the test joins it via defer, so t.Logf
can no longer run after the test function has returned.

No production code changed. No comment changes.

Per CONTRIBUTING's "one component per commit", this should land as two commits — one for CubeMaster, one
for Cubelet.

Testing

CubeMaster:
  ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/queueworker  10.694s
  ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/recov         5.869s

Cubelet:
  ok  github.com/tencentcloud/CubeSandbox/Cubelet/pkg/queueworker  12.305s
  ok  github.com/tencentcloud/CubeSandbox/Cubelet/pkg/recov         5.478s

all under -race -count=1, where master reports --- FAIL plus a WARNING: DATA RACE for
TestQueueBlock and TestGoWithRetryWithCrash.

Module-wide go test -race ./... in CubeMaster:

	races
master	9
this branch	7

The remaining 7 are the localcache production races, which are fixed on their own branch. The two
changes together take CubeMaster to 0.

On the bufferqueue one

I could not reproduce that race in five isolated runs of the package — it showed up only in a full
go test -race ./.... bw.Workings() is correctly atomic.LoadInt32, so the plausible cause is the
unjoined goroutine touching t after the test returned, which is a testing misuse regardless of
whether the detector catches it on a given run. I fixed it on those grounds rather than claiming a
verified red/green, since I would rather say so than present an unproven fix as proven.

CI gates checked locally:

• gofmt -l on all four packages — clean (fmt-check).
• go test -race on all four — clean.

[Copilot]

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

[cubesandboxbot]

Minor (pre-existing, but this PR rewrites the test): this pre-Push check is timing-dependent — got.Load() is evaluated immediately after the goroutine is spawned, with no guarantee the goroutine has been scheduled yet. If BPop ever regresses to return immediately, the test can still pass: this check often runs before the goroutine stores true, and by the time done closes, if !got.Load() below also sees true. The done channel now reliably joins the goroutine (good), but the "should block when queue is empty" assertion is still not a strong check. (Also applies to the identical Cubelet copy.)

[cubesandboxbot]

Review — PR #1479: tests: fix the data races in queueworker, recov and bufferqueue

AI-generated review. Reviewed against the base branch tree (master) using the prefetched pr.diff and meta.json; no PR head checkout.

Overall verdict: LGTM — approve. This is a test-only PR (5 files, +29/−12, no production code changed) that fixes real go test -race failures in three packages. I traced each change against the production code it exercises; all three fixes are correct, idiomatic, and don't introduce new races or deadlocks.

Change-by-change analysis

1. queueworker/queue_test.go — TestQueueBlock (CubeMaster + Cubelet)

got becomes atomic.Bool and a done channel joins the goroutine. Correct:

• The deferred close(done) runs after got.Store(true), so receiving from done establishes a happens-before edge that guarantees the subsequent got.Load() returns true.
• The race between the goroutine's write and the main goroutine's reads is eliminated.
• Replacing the fixed 1s time.Sleep with a 5s select/time.After deadline is an improvement: the test finishes as soon as BPop returns, and a non-returning BPop now fails with a clear t.Fatal message instead of a confusing assertion.

Verified against queue.go: BPop is <-q.queueCh (blocking receive), Push is a non-blocking send on a 5-buffered channel, so BPop unblocks immediately after the single push. Queue size 5 guarantees Push can't fail.

2. recov/runtime_test.go — TestGoWithRetryWithCrash (CubeMaster + Cubelet)

int(panicTime) → int(atomic.LoadInt32(&panicTime)). Correct: production WithRetry runs the panic-handler goroutine which writes panicTime via atomic.AddInt32 (in runtime.go), so the direct read was a genuine data race. The atomic load fixes it. The sibling TestGoWithRetryWithoutCrash still reads panicTime directly, but that's safe — its retry handler never runs (the function doesn't panic), so there is no concurrent write.

3. bufferqueue/bufferqueue_test.go — TestQueueCheckTimestampPriority (CubeMaster only)

The watcher goroutine is joined via watcherDone, with defer func() { <-watcherDone }(). Correct and I specifically checked for deadlock:

• The watcher's only blocking call is a bounded time.Sleep(1s); every other path is non-blocking.
• Its loop always terminates: either worked == testnum (set once the queue drains) or the 30s ctx expires — so the deferred receive always unblocks.
• The defer ordering is safe: <-watcherDone runs before defer cancel(), but by the time the defers execute, GraceFullStop has already either drained the queue (→ watcher exits on worked == testnum) or the ctx has fired (→ watcher exits on ctx.Done()).
• This does guarantee the intended property: t.Logf can no longer run after the test function returns, since the test can't return until the watcher has exited.

This also resolves the "unreproducible in isolation" case the author describes: the fix is justified on the grounds of testing.T misuse (logging from an unjoined goroutine after test completion) rather than a reproduced race, which is the right call.

Minor observations (none blocking)

• TestQueueBlock's "BPop should block when queue is empty" check remains timing-dependent — got.Load() is read immediately after the goroutine is spawned, with no scheduling guarantee, so a regressed non-blocking BPop could slip past both checks and the test could still pass. Pre-existing (the original had the same shape), and noted inline on the CubeMaster copy; applies to the identical Cubelet copy too.
• recov tests still join the retry goroutine with a fixed time.Sleep(2 * time.Second) rather than a channel/WaitGroup. Not a race (the atomic read is safe), but a pre-existing flake source. Out of scope for this PR.
• Nit: the t.Errorf message in TestGoWithRetryWithCrash hardcodes "should be %d, actual %d", 0, 1 regardless of the actual values (pre-existing, unchanged).

CI/portability notes

• atomic.Bool requires Go ≥ 1.19; CubeMaster/go.mod declares go 1.25.7 and Cubelet/go.mod go 1.24.8, so both are fine.
• The diff is gofmt-clean (verified by inspection of the added blocks).
• No review-input/TRUNCATED marker present, so the diff is complete.

Conclusion: The PR does what it claims — removes three test-side data races, none in production code — with correct synchronization. Happy to approve; the inline note and the observations above are optional polish, not blockers.