fix: add LRU eviction to unbounded caches to prevent memory leaks#33
Open
ggfevans wants to merge 3 commits intosteipete:mainfrom
Open
fix: add LRU eviction to unbounded caches to prevent memory leaks#33ggfevans wants to merge 3 commits intosteipete:mainfrom
ggfevans wants to merge 3 commits intosteipete:mainfrom
Conversation
The app accumulated ~2GB memory after running for 2.5 days due to unbounded in-memory caches that never evicted old entries. Changes: - ETagCache: Add LRU eviction with 200 entry limit. Tracks access order and evicts least recently used entries when capacity is reached. - RecentListCache: Add LRU eviction with 50 entry limit per cache type. There are 9 cache instances (issues, PRs, releases, commits, etc.) that now evict least recently accessed repository entries. Uses access-order array for O(1) eviction (same pattern as ETagCache). - Add @mainactor to RecentListCache for explicit thread safety. - Add clearAllCaches() method to RecentMenuService to clear all caches including the recentCommitCounts dictionary. The limits are conservative (200 ETag entries, 50 repos per list type) to maintain good cache hit rates while preventing unbounded growth. Fixes steipete#13 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ggfevans
force-pushed
the
fix/13-memory-leak-unbounded-caches
branch
from
0a5721c to
7e1ecf7
Compare
CodeRabbit identified that the ETagCache save() method would unnecessarily evict entries when updating an existing key at capacity. This fix ensures eviction only occurs when inserting new keys, matching the pattern in RecentListCache. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Author
|
Hey @steipete I'm curious on your thoughts here in terms of cache count thresholds. Otherwise this seemed relatively low hanging as well in terms of closing potential memory leaks/unbounded growth. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ETagCachewith 200 entry limitRecentListCachewith 50 entry limit per cache type@MainActortoRecentListCachefor explicit thread safetyclearAllCaches()method toRecentMenuServicefor manual cache clearingProblem
After running for ~2.5 days, RepoBar accumulated ~2GB of memory due to unbounded in-memory caches:
(etag, Data)tuples for every URL ever fetched with no evictionSolution
Add LRU (Least Recently Used) eviction to both cache types using an access-order tracking array:
ETagCache
RecentListCache
@MainActorisolation for thread safetyThe limits are conservative to maintain good cache hit rates while preventing unbounded growth.
Test plan
swift build)swift test)Fixes #13
🤖 Generated with Claude Code