[TT-13939] Embed memorycache, drop leakybucket import

[titpetric]

User description

TT-13919

Summary	[Testing/memory leak] Fix memorycache leak and embed package to gw
Type	Bug
Status	In Dev
Points	N/A
Labels	-

---

Leakybucket repo last change was 8 years ago. Due to a goroutine leak in memory cache, it was better to clean it up a bit and embed it into the gateway, so we can archive that repository. It amounts mostly to a test fix, as the leak impacts test runs and benchmarks.

https://tyktech.atlassian.net/browse/TT-13919

---

PR Type

Bug fix, Enhancement, Tests

---

Description

• Embedded memorycache package into the gateway, replacing leakybucket.

• Introduced a new in-memory cache system with auto-expiry.

• Added thread-safe bucket storage and cache management.

• Removed outdated leakybucket dependency from the project.

---

Changes walkthrough 📝

	Relevant files
Enhancement	6 files session_manager.go Replace leakybucket with embedded memorycache in session management +4/-7      bucket.go Add bucket implementation for in-memory rate limiting        +45/-0    cache.go Introduce thread-safe in-memory cache with auto-expiry      +90/-0    item.go Implement cache item with expiration handling                        +32/-0    storage.go Add in-memory bucket storage for rate limiting                      +37/-0    bucket.go Define bucket interface and error handling for rate limiting +40/-0
Tests	1 files cache_test.go Add basic test for cache shutdown functionality                    +15/-0
Documentation	1 files doc.go Add documentation for internal data model package                +2/-0
Dependencies	2 files go.mod Remove `leakybucket` dependency from module                            +0/-1      go.sum Update dependencies to reflect removal of `leakybucket`    +0/-2

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[buger]

I'm a bot and I 👍 this PR title. 🤖

[buger]

I'm a bot and I 👍 this PR title. 🤖

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Memory Leak The startCleanupTimer function in the Cache implementation spawns a goroutine for cleanup but does not ensure proper termination or cleanup of resources when the context is canceled. This could lead to resource leaks if not handled correctly. func (cache *Cache) startCleanupTimer(ctx context.Context) { interval := cache.ttl if interval < time.Second { interval = time.Second } ticker := time.NewTicker(interval) defer ticker.Stop() for { select { case <-ctx.Done(): // fmt.Println("Shutting down cleanup timer:", ctx.Err()) return case <-ticker.C: cache.cleanup() } } Thread Safety Concern The Add method in the Bucket implementation uses a mutex for thread safety, but there is no explicit test coverage to validate its behavior under concurrent access scenarios. func (b *Bucket) Add(amount uint) (model.BucketState, error) { b.mutex.Lock() defer b.mutex.Unlock() if time.Now().After(b.reset) { b.reset = time.Now().Add(b.rate) b.remaining = b.capacity } if amount > b.remaining { return model.BucketState{Capacity: b.capacity, Remaining: b.remaining, Reset: b.reset}, model.ErrBucketFull } b.remaining -= amount return model.BucketState{Capacity: b.capacity, Remaining: b.remaining, Reset: b.reset}, nil Insufficient Test Coverage The Cache implementation lacks comprehensive test coverage for critical methods like Set, Get, and cleanup, which are essential for ensuring correctness and reliability. // Set is a thread-safe way to add new items to the map func (cache *Cache) Set(key string, data *Bucket) { cache.mutex.Lock() item := &Item{data: data} item.touch(cache.ttl) cache.items[key] = item cache.mutex.Unlock() } // Get is a thread-safe way to lookup items // Every lookup, also touches the item, hence extending it's life func (cache *Cache) Get(key string) (data *Bucket, found bool) { cache.mutex.Lock() item, exists := cache.items[key] if !exists || item.expired() { data = &Bucket{} found = false } else { item.touch(cache.ttl) data = item.data found = true } cache.mutex.Unlock() return } // Count returns the number of items in the cache // (helpful for tracking memory leaks) func (cache *Cache) Count() int { cache.mutex.RLock() count := len(cache.items) cache.mutex.RUnlock() return count } func (cache *Cache) cleanup() { cache.mutex.Lock() for key, item := range cache.items { if item.expired() { delete(cache.items, key) } } cache.mutex.Unlock()

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Validate parameters in Create method Add a check in the Create method to ensure that the capacity and rate parameters are valid (e.g., non-zero and positive) before creating a new bucket. internal/memorycache/storage.go [22-36] func (s *BucketStorage) Create(name string, capacity uint, rate time.Duration) (model.Bucket, error) { + if capacity == 0 || rate <= 0 { + return nil, errors.New("invalid capacity or rate") + } b, ok := s.buckets.Get(name) if ok { return b, nil } b = &Bucket{ capacity: capacity, remaining: capacity, reset: time.Now().Add(rate), rate: rate, } s.buckets.Set(name, b) return b, nil } Suggestion importance[1-10]: 9 Why: Adding validation for capacity and rate parameters in the Create method ensures that invalid inputs are caught early, preventing potential logical errors or unexpected behavior. This is a critical improvement for input validation.	9
	Add nil-check validation in Set method Ensure that the Set method in the Cache struct checks for nil or invalid Bucket data before adding it to the cache to prevent potential runtime errors. internal/memorycache/cache.go [28-33] func (cache *Cache) Set(key string, data *Bucket) { + if data == nil { + return + } cache.mutex.Lock() item := &Item{data: data} item.touch(cache.ttl) cache.items[key] = item cache.mutex.Unlock() } Suggestion importance[1-10]: 8 Why: Adding a nil-check in the Set method is a valid improvement to prevent runtime errors caused by invalid data. This change enhances the robustness of the code without introducing any side effects.	8
General	Use read lock for Get method Optimize the Get method by using a read lock (RLock) instead of a full lock (Lock) for checking and retrieving items, as it improves concurrency for read operations. internal/memorycache/cache.go [37-50] func (cache *Cache) Get(key string) (data *Bucket, found bool) { - cache.mutex.Lock() + cache.mutex.RLock() item, exists := cache.items[key] + cache.mutex.RUnlock() if !exists || item.expired() { data = &Bucket{} found = false } else { item.touch(cache.ttl) data = item.data found = true } - cache.mutex.Unlock() return } Suggestion importance[1-10]: 7 Why: Using a read lock (RLock) for retrieving items in the Get method improves concurrency and performance for read operations. However, the suggestion does not address the fact that item.touch modifies the item, which requires a write lock, making the suggestion partially correct.	7
	Add panic recovery in cleanup function Ensure the cleanup function in the Cache struct handles potential panics during deletion of expired items to avoid crashing the application. internal/memorycache/cache.go [62-69] func (cache *Cache) cleanup() { + defer func() { + if r := recover(); r != nil { + // Log or handle the panic + } + }() cache.mutex.Lock() for key, item := range cache.items { if item.expired() { delete(cache.items, key) } } cache.mutex.Unlock() } Suggestion importance[1-10]: 6 Why: Adding panic recovery in the cleanup function is a reasonable safeguard to prevent application crashes due to unexpected errors during deletion. However, the likelihood of a panic in this context is low, making the improvement less critical.	6

[github-actions]

API Changes

no api changes detected

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.5% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarQube Cloud