Skip to content

fix(auth): add Close() to RBACManager and bound permission caches (#295, #296)#381

Merged
xe-nvdk merged 2 commits intomainfrom
fix/rbac-goroutine-leak-unbounded-cache
Apr 10, 2026
Merged

fix(auth): add Close() to RBACManager and bound permission caches (#295, #296)#381
xe-nvdk merged 2 commits intomainfrom
fix/rbac-goroutine-leak-unbounded-cache

Conversation

@xe-nvdk
Copy link
Copy Markdown
Member

@xe-nvdk xe-nvdk commented Apr 9, 2026

Summary

Test plan

  • go build ./cmd/... ./internal/... passes
  • go test ./internal/auth/... passes
  • Post-implementation security review: RBAC is license-gated, no race conditions in Close(), eviction is safe (re-checks hit database)

Closes #295
Closes #296

…, #296)

Two related RBAC fixes for Enterprise GA:

1. Goroutine leak (#295): cacheCleanupLoop ran in an infinite loop with
   no shutdown mechanism. Added a done channel and Close() method
   following the AuthManager pattern. RBACManager is now registered with
   the shutdown coordinator at PriorityAuth.

2. Unbounded caches (#296): both tokenCache and permCache had no maximum
   size — only TTL-based expiration. Added configurable MaxCacheSize
   (default 10,000 entries per cache) with random eviction on insertion
   when the cache exceeds its limit.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request fixes a goroutine leak in RBACManager by adding a Close method and a shutdown signal, and prevents unbounded memory growth by implementing a maximum cache size with random eviction. Feedback was provided to extract the duplicated eviction logic into a reusable helper method to improve code maintainability.

Comment thread internal/auth/rbac_manager.go Outdated
Comment on lines +935 to +940
if len(rm.permCache) >= rm.maxCacheSize {
for k := range rm.permCache {
delete(rm.permCache, k)
break
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This cache eviction logic is duplicated in CheckPermissionsBatch (lines 1059-1064) and for tokenCache in getTokenRBACData (lines 1142-1147). To improve maintainability and reduce code duplication, consider extracting this logic into a private helper method. This would centralize the eviction strategy, making future modifications (e.g., switching to an LRU policy) much simpler.

Extracted evictPermCacheIfFull() and evictTokenCacheIfFull() to
centralize the eviction strategy across the 3 insertion points.
Makes future changes (e.g. switching to LRU) a single-point modification.
@xe-nvdk xe-nvdk merged commit 92b8e3f into main Apr 10, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

high(auth): RBAC permission caches grow unbounded high(auth): RBACManager goroutine leak — no Close() method

1 participant