cubemaster: clamp the gRPC pool refcount in the stored value, not a local copy

[dwin-gharibi]

Closes #1474.

Motivation

Both incrRef and decrRef clamped a local int32 after the atomic add, so neither clamp had
any effect on p.ref:

newRef := atomic.AddInt32(&p.ref, 1)
if newRef >= math.MaxInt32 { newRef = math.MaxInt32 }   // local only

staticcheck flags the increment side as SA4003. The consequential half is the decrement: a single
unbalanced decrRef drives the stored counter to -1, and from then on the
atomic.LoadInt32(&p.ref) == 0 guard inside decrRef never holds again — so deleteFrom(MaxIdle) is
never reached and idle connections above MaxIdle are never reclaimed. A silent connection leak.

What this changes

CubeMaster/pkg/base/grpc-middleware/pool/pool.go — both operations become compare-and-swap loops so
the bound is enforced on the stored value:

• incrRef saturates at math.MaxInt32 instead of wrapping negative.
• decrRef stops at 0 instead of going negative, which keeps the idle-trim branch reachable.

decrRef's existing idle-trim block is unchanged; it now sees a counter that can actually reach 0.

No comment changes.

Testing

New: CubeMaster/pkg/base/grpc-middleware/pool/pool_refcount_test.go

• TestIncrRefDoesNotOverflowPastMaxInt32 — asserts the stored value saturates.
• TestIncrRefIncrementsNormally — normal path unchanged.
• TestDecrRefDoesNotGoNegative — repeated unbalanced releases leave the stored value at 0.
• TestDecrRefDecrementsNormally — normal path unchanged.
• TestRefCountIsBalancedUnderConcurrency — 64 goroutines × 500 balanced incr/decr pairs end at 0,
  which exercises the CAS retry loops.

Red/green verified — with pool.go reverted to master:

--- FAIL: TestIncrRefDoesNotOverflowPastMaxInt32
--- FAIL: TestDecrRefDoesNotGoNegative
FAIL

and with the fix:

ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/grpc-middleware/pool  0.283s

Also clean under the race detector:

$ go test -race -count=1 ./pkg/base/grpc-middleware/pool/
ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/grpc-middleware/pool  1.740s

CI gates checked locally:

• gofmt -l on the package — clean (fmt-check).
• GOOS=linux go build on the package — clean.
• staticcheck -checks 'SA*' — the SA4003 finding is gone; the remaining SA1019 hits are
  pre-existing grpc.DialContext / WithInsecure / WithBlock deprecations in options.go, not
  touched here.

[Copilot]

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

[cubesandboxbot]

Minor behavior change worth a thought: when p.ref is already 0, this loop breaks at cur <= 0 with newRef still at its zero value — so the idle-trim block below (newRef == 0 && atomic.LoadInt32(&p.current) > MaxIdle) fires even though no decrement actually occurred. Under the old code the stored value went to −1 in that case, so the inner atomic.LoadInt32(&p.ref) == 0 guard always blocked the trim on an unbalanced release.

The result is benign today (the trim is idempotent and only proceeds when p.ref is genuinely 0, and the redundant-trim window with current > MaxIdle is tiny), but the code can no longer distinguish "decremented to zero" from "was already zero." If that distinction ever matters, track success with a bool (e.g. decremented := false) and only run the trim branch when a decrement really happened.

[cubesandboxbot]

Review: cubemaster: clamp the gRPC pool refcount in the stored value, not a local copy (#1475)

AI-generated review — verified against the base-branch workspace. Not a human approval.

Verdict

The PR fixes a real bug. In the old code both incrRef and decrRef clamped a local copy after atomic.AddInt32, so the stored p.ref wrapped negative on overflow, and once negative the atomic.LoadInt32(&p.ref) == 0 guard inside decrRef never held again — the idle-trim branch became permanently unreachable and connections above MaxIdle leaked. The CAS-loop rewrite enforces the bound on the stored value and the invariant p.ref ∈ [0, MaxInt32] now holds. staticcheck SA4003 is correctly eliminated, tests are reasonable, and red/green + race runs are documented.

No correctness bug found in the new code itself. Three non-blocking items below.

Findings

1. [Medium] The identical bug is left unfixed in the Cubelet copy
Cubelet/pkg/grpc-middleware/pool/pool.go:114–141 contains a verbatim copy of the old incrRef/decrRef (same atomic.AddInt32 + local-only clamp). No importers of that package were found in the Cubelet module during this review, so it may currently be dead code — but if it is ever compiled and used, the same silent connection-leak applies. Either apply the same fix there or note the duplication deliberately.

2. [Low] decrRef now triggers the idle-trim on an "already zero" release (inline comment at pool.go:134)
When p.ref is already 0, the CAS loop breaks immediately with newRef at its zero value, so the newRef == 0 && current > MaxIdle branch runs even though no decrement happened. Benign today (trim is idempotent and only proceeds when p.ref is genuinely 0), but the code can no longer distinguish "decremented to zero" from "was already zero." Consider tracking whether a decrement actually succeeded.

3. [Low] Test gap: no test exercises the idle-trim path
The tests verify the counter clamps and balances, but none set current > MaxIdle and assert that a decrement to 0 triggers deleteFrom/current reset — which is precisely the branch the PR exists to keep reachable. A test like newRefProbe with current set above MaxIdle (the helper only sets MaxIdle: 1; current defaults to 0, so the trim condition is never reachable in any current test) would lock in the fix's core behavior.

Minor notes

• Get() still calls incrRef() before the current == 0 closed-check and never balances it on the ErrClosed path — a pre-existing leak, not introduced here; the saturation actually makes it strictly less harmful than the old wrap-around.
• The redundant-trim case (two concurrent decrRefs where the second sees 0) calls deleteFrom twice under the lock; deleteFrom/reset are idempotent, so this is harmless.