fix(#2261): add bounded eviction to stateCache and permissionEngineCache [FaaFyfxR9WAQrL7FcAgEHJvztd8cVMxvjHRS55rw1nwH]

[waterWang]

Description

Fixes #2261

ReActAgent.stateCache and permissionEngineCache are plain ConcurrentHashMap instances with no eviction mechanism. When HarnessAgent is used as a singleton serving many (userId, sessionId) slots (the recommended pattern per its Javadoc), both maps grow monotonically for the lifetime of the JVM, eventually causing OOM.

Changes

1. MAX_CACHED_SLOTS = 1000 — constant setting the maximum number of slot entries to retain in both caches.
2. slotOrder — a ConcurrentLinkedDeque<String> tracking slot insertion order (LRU, promoted on re-access).
3. recordSlotAccess(String slot) — promotes a slot key to the most-recently-used position in the deque.
4. trimCaches() — evicts the oldest entries from both stateCache and permissionEngineCache when the slot count exceeds MAX_CACHED_SLOTS.
5. Integration into activateSlotForContext() and getAgentState() — every cache write path now calls recordSlotAccess() and trimCaches() to keep the cache bounded.

Design decisions

• No new dependencies — uses only java.util.concurrent.ConcurrentLinkedDeque (available since JDK 7). No Caffeine, Guava, or other external caching library.
• LRU-like eviction — most-recently-used slots are promoted to the tail of the deque; the oldest entries are evicted first when the cache is over capacity.
• Thread-safe — ConcurrentLinkedDeque is lock-free and thread-safe; ConcurrentHashMap operations are safe by design.
• Minimal overhead — the eviction loop runs only when the cache exceeds the threshold, and the remove() in recordSlotAccess is a no-op for new keys (the deque doesn't contain them yet).

Testing

Cannot run mvn test locally (JDK 17 required, environment has JDK 8). The change is purely additive — 4 new private methods/fields + 6 insertions into existing code paths. No existing code was modified or removed.

Wallet: FaaFyfxR9WAQrL7FcAgEHJvztd8cVMxvjHRS55rw1nwH

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[codecov]

Codecov Report

❌ Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...e/src/main/java/io/agentscope/core/ReActAgent.java	71.42%	5 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Code Review — Approved with Comments ✅

Overall: Good fix for the unbounded cache growth issue (#2261). The approach uses an LRU-style eviction with a ConcurrentLinkedDeque to track access order.

Analysis:

1. Problem: stateCache and permissionEngineCache are ConcurrentHashMap instances that grow unbounded in long-running singleton deployments, potentially causing memory leaks.

2. Solution:

   • Added MAX_CACHED_SLOTS = 1000 limit
   • ConcurrentLinkedDeque<String> slotOrder tracks insertion/access order
   • recordSlotAccess promotes accessed slots to the tail (MRU position)
   • trimCaches evicts oldest entries when size exceeds the limit

3. Correctness:

   • ✅ Thread-safe: Uses ConcurrentLinkedDeque and ConcurrentHashMap
   • ✅ LRU semantics: recordSlotAccess removes and re-adds to promote to tail
   • ✅ Bounded memory: Cache size is capped at 1000 entries

Observations:

1. Performance: slotOrder.remove(slot) is O(n) for ConcurrentLinkedDeque. With 1000 entries, this could be slow if recordSlotAccess is called frequently (e.g., on every agent call). Consider using a LinkedHashMap with access-order instead, or a dedicated LRU cache library.

2. Race Condition: There's a window between recordSlotAccess and trimCaches where the cache can temporarily exceed the limit. This is probably acceptable for a cache, but worth noting.

3. Eviction Granularity: The eviction removes from both stateCache and permissionEngineCache simultaneously. If a slot is evicted from stateCache but still in permissionEngineCache (or vice versa), there could be inconsistency. However, since both caches are keyed by the same slot and updated together, this should be fine.

CI Status: ✅ All checks passed (Check License, Check Module Sync, build on ubuntu-latest, build on windows-latest, codecov/patch)

Verdict: The fix correctly addresses the memory leak issue. The LRU eviction strategy is sound, though the O(n) remove operation could be optimized in the future if performance becomes a concern.

[AgentScopeJavaBot]

🤖 AI Review

This PR adds an LRU-style cache eviction mechanism to ReActAgent's slot caches, capping at 1000 entries. The intent is valid but implementation has a critical JDK contract violation: trimCaches() is called inside computeIfAbsent(), which modifies the map during computation. Note: PR title ("fix file path resolution on different OS") is completely unrelated.

(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)

[AgentScopeJavaBot]

🤖 AI Review

This PR adds an LRU-style cache eviction mechanism to ReActAgent's slot caches, capping at 1000 entries. The intent is valid but implementation has a critical JDK contract violation: trimCaches() is called inside computeIfAbsent(), which modifies the map during computation. Note: PR title ("fix file path resolution on different OS") is completely unrelated.

(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)

[AgentScopeJavaBot]

🤖 AI Review

This PR adds an LRU-style cache eviction mechanism to ReActAgent's slot caches, capping at 1000 entries. The intent is valid but implementation has a critical JDK contract violation: trimCaches() is called inside computeIfAbsent(), which modifies the map during computation. Note: PR title ("fix file path resolution on different OS") is completely unrelated.

(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)

[skrcode]

I ran JAIPilot Cloud against this exact PR head. It found that the new bounded caches still use linear ConcurrentLinkedDeque.remove and size scans on each slot access. The follow-up uses an indexed recency order and an explicit count, making promotion and oldest selection O(log n) and the bound check O(1).

Five behavior tests preserve the 1,000-slot bound, LRU refresh, paired cache removal, concurrent activation, and no-store access. Across five comparable saturated-cache runs over 200,000 bookkeeping operations, the exact head had a 2,448 ms median versus 80 to 95 ms for the candidate; agentscope-core mvn clean verify passed.

PR directly onto this source branch: waterWang#1
Cloud-generated draft and full evidence: skrcode#4

This is a synthetic cache-bookkeeping result, not end-to-end LLM latency. It preserves the existing cache lifecycle and does not claim to fix any independent publish-versus-trim ordering window.