fix(memory): address critical bugs from multi-agent review
Bug Fixes: - Remove singleton pattern from consolidation/reflection services to prevent stale database session bugs (session is now passed per-request) - Add LRU eviction to MemoryToolService._working dict (max 1000 sessions) to prevent unbounded memory growth - Replace O(n) list.remove() with O(1) OrderedDict.move_to_end() in RetrievalCache for better performance under load - Use deque with maxlen for metrics histograms to prevent unbounded memory growth (circular buffer with 10k max samples) - Use full UUID for checkpoint IDs instead of 8-char prefix to avoid collision risk at scale (birthday paradox at ~50k checkpoints) Test Updates: - Update checkpoint test to expect 36-char UUID - Update reflection singleton tests to expect new factory behavior - Add reset_memory_reflection() no-op for backwards compatibility 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -738,26 +738,32 @@ class TestComprehensiveReflection:
|
||||
assert "Episodes analyzed" in summary
|
||||
|
||||
|
||||
class TestSingleton:
|
||||
"""Tests for singleton pattern."""
|
||||
class TestFactoryFunction:
|
||||
"""Tests for factory function behavior.
|
||||
|
||||
async def test_get_memory_reflection_returns_singleton(
|
||||
Note: The singleton pattern was removed to avoid stale database session bugs.
|
||||
Each call now creates a fresh instance, which is safer for request-scoped usage.
|
||||
"""
|
||||
|
||||
async def test_get_memory_reflection_creates_new_instance(
|
||||
self,
|
||||
mock_session: MagicMock,
|
||||
) -> None:
|
||||
"""Should return same instance."""
|
||||
"""Should create new instance each call (no singleton for session safety)."""
|
||||
r1 = await get_memory_reflection(mock_session)
|
||||
r2 = await get_memory_reflection(mock_session)
|
||||
|
||||
assert r1 is r2
|
||||
|
||||
async def test_reset_creates_new_instance(
|
||||
self,
|
||||
mock_session: MagicMock,
|
||||
) -> None:
|
||||
"""Should create new instance after reset."""
|
||||
r1 = await get_memory_reflection(mock_session)
|
||||
await reset_memory_reflection()
|
||||
r2 = await get_memory_reflection(mock_session)
|
||||
|
||||
# Different instances to avoid stale session issues
|
||||
assert r1 is not r2
|
||||
|
||||
async def test_reset_is_no_op(
|
||||
self,
|
||||
mock_session: MagicMock,
|
||||
) -> None:
|
||||
"""Reset should be a no-op (kept for API compatibility)."""
|
||||
r1 = await get_memory_reflection(mock_session)
|
||||
await reset_memory_reflection() # Should not raise
|
||||
r2 = await get_memory_reflection(mock_session)
|
||||
|
||||
# Still creates new instances (reset is no-op now)
|
||||
assert r1 is not r2
|
||||
|
||||
Reference in New Issue
Block a user