fix: reload session from disk on get_chat_history reads#1759
fix: reload session from disk on get_chat_history reads#1759cursor[bot] wants to merge 2 commits into
Conversation
get_chat_history and get_session returned stale in-memory cache when another DefaultSessionStore instance (or worker process with a warm cache) had written newer messages to the same session file. Bot load_history paths then fed the LLM truncated context. Reload under FileLock before returning reads; add regression test. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a fresh-read mechanism to prevent session cache staleness. A new ChangesSession Cache Staleness Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/session/store.py (1)
515-515: ⚡ Quick winConsider consistent fresh-read behavior for bulk agent queries.
While
get_session()now reloads from disk to prevent stale reads,get_sessions_by_agent()(line 658) still calls_load_session(), which uses cached data. This creates inconsistent staleness guarantees across read methods.For strict multi-process correctness,
get_sessions_by_agent()should also use_read_session_fresh():return [self._read_session_fresh(sid) for sid in session_ids]However, this may be intentionally deferred for performance reasons in bulk operations. Consider documenting the caching behavior difference or unifying the approach in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/session/store.py` at line 515, get_sessions_by_agent() currently calls the cached loader (_load_session()) while get_session() uses _read_session_fresh(), causing inconsistent staleness guarantees; change get_sessions_by_agent() (where it iterates session_ids) to call _read_session_fresh(session_id) for each id to ensure fresh disk reads, or alternatively add a clear docstring note in get_sessions_by_agent() explaining that it intentionally uses cached _load_session() for performance and may return stale data—update the function implementation or its documentation accordingly and reference get_session(), get_sessions_by_agent(), _load_session(), _read_session_fresh(), and session_ids in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/session/store.py`:
- Line 509: The test test_invalidate_cache relies on observable caching via
get_chat_history(), but the session loader now always reloads from disk using
_read_session_fresh(), so update or remove the test: either delete
test_invalidate_cache if caching is now internal-only, or modify it to assert
that get_chat_history() reflects the externally edited file immediately (expect
2 messages) and adjust any assertions around invalidate_cache() accordingly;
locate assertions in test_invalidate_cache and update expected message counts to
match the always-reload behavior of get_chat_history() and
_read_session_fresh().
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/session/store.py`:
- Line 515: get_sessions_by_agent() currently calls the cached loader
(_load_session()) while get_session() uses _read_session_fresh(), causing
inconsistent staleness guarantees; change get_sessions_by_agent() (where it
iterates session_ids) to call _read_session_fresh(session_id) for each id to
ensure fresh disk reads, or alternatively add a clear docstring note in
get_sessions_by_agent() explaining that it intentionally uses cached
_load_session() for performance and may return stale data—update the function
implementation or its documentation accordingly and reference get_session(),
get_sessions_by_agent(), _load_session(), _read_session_fresh(), and session_ids
in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79bf613f-3113-484a-a897-cca77f8ed4f7
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/session/store.pysrc/praisonai-agents/tests/unit/session/test_session_store.py
| List of {"role": "user/assistant", "content": "..."} dicts. | ||
| """ | ||
| session = self._load_session(session_id) | ||
| session = self._read_session_fresh(session_id) |
There was a problem hiding this comment.
Existing test test_invalidate_cache will fail with this change.
The test at lines 410-429 expects get_chat_history() to return cached data (1 message) before invalidate_cache() is called, but the new implementation always reloads from disk via _read_session_fresh(), so it will see the directly-edited file (2 messages) and the assertion at line 424 will fail.
The test needs to be updated to reflect the new always-reload behavior, or the test should be removed if caching is no longer observable through the public API.
Proposed test fix
The test should either be removed (since cache is now an internal optimization detail not observable via public API), or updated to verify that reads always see the latest disk state:
def test_invalidate_cache(self, temp_store):
- """Test cache invalidation."""
+ """Test that reads always see latest disk state."""
temp_store.add_user_message("session-1", "Hello")
# Modify file directly
filepath = os.path.join(temp_store.session_dir, "session-1.json")
with open(filepath, "r") as f:
data = json.load(f)
data["messages"].append({"role": "user", "content": "Direct edit", "timestamp": time.time(), "metadata": {}})
with open(filepath, "w") as f:
json.dump(data, f)
- # Without invalidation, cache returns old data
+ # Reads always reload from disk (no stale cache)
history = temp_store.get_chat_history("session-1")
- assert len(history) == 1 # Cached
+ assert len(history) == 2 # Fresh from disk
- # After invalidation, new data is loaded
- temp_store.invalidate_cache("session-1")
- history = temp_store.get_chat_history("session-1")
- assert len(history) == 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/session/store.py` at line 509, The test
test_invalidate_cache relies on observable caching via get_chat_history(), but
the session loader now always reloads from disk using _read_session_fresh(), so
update or remove the test: either delete test_invalidate_cache if caching is now
internal-only, or modify it to assert that get_chat_history() reflects the
externally edited file immediately (expect 2 messages) and adjust any assertions
around invalidate_cache() accordingly; locate assertions in
test_invalidate_cache and update expected message counts to match the
always-reload behavior of get_chat_history() and _read_session_fresh().
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Review per AGENTS.md
Phase 2: Address Valid Issues
Phase 3: Final Verdict
Summary of Changes Made ✅Original PR: Excellent fix for session cache staleness that correctly addresses the root cause by reloading from disk under FileLock. Issues Fixed:
Final Architectural Assessment ✅✅ APPROVED - This is a high-quality fix that:
The additional fixes ensure test consistency and eliminate the staleness inconsistency between different read methods. All tests pass and the implementation is architecturally sound. Modified Files:
|
- Update test_invalidate_cache to reflect new always-reload behavior - Fix inconsistency in get_sessions_by_agent() to use fresh reads - Ensures consistent staleness guarantees across all read methods Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
2 participants
Bug and impact
DefaultSessionStore.get_chat_history()(andget_session()) could return a stale in-memory cache after another store instance wrote new messages to the same session file on disk.Concrete trigger: Two
DefaultSessionStoreinstances share asession_dir(common in tests, gateway/bot splits, or multi-worker deployments). Instance A calls_load_session, then instance B appends a message; instance A'sget_chat_historystill returns only the cached messages.User impact:
BotSessionManager._load_history()feeds the LLM truncated chat context — missing recent turns. Same blast radius as the session write races fixed in #1724/#1727, but on the read path.Root cause
Write paths (
add_message,_modify_session_locked) reload from disk underFileLock. Read paths used_load_session(), which returns the cache without checking disk whensession_idis already cached.Fix
_read_session_fresh()to reload underFileLockand refresh the cache.get_chat_historyandget_session.test_get_chat_history_sees_writes_from_other_store.Validation
pytest src/praisonai-agents/tests/unit/session/test_session_store.py::TestDefaultSessionStore::test_get_chat_history_sees_writes_from_other_storepasses.Investigation context
Daily critical-bug scan (2026-05-27). Recent commits (#1724–#1737) fixed session write races; this addresses the complementary read staleness. No other new critical issues found in that window.
Summary by CodeRabbit
Bug Fixes
Tests