fix: reload session from disk on get_chat_history reads#1759
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>
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? |
Greptile SummaryThis PR fixes read-path staleness in
Confidence Score: 4/5Safe to merge; the fix correctly eliminates stale-read staleness on the read path and the new test validates the scenario end-to-end. The core logic is sound — replacing the cache-first path with a disk-always read under FileLock is the right approach for the described multi-instance problem. Two non-blocking concerns remain: a narrow window between the FileLock release and the in-process lock acquisition where a concurrent write could leave the cache in a slightly older state (harmless in practice since no production code calls store.py — specifically Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant StoreA as DefaultSessionStore A
participant StoreB as DefaultSessionStore B
participant FileLock
participant Disk
Note over StoreA,StoreB: Before fix — stale cache
StoreB->>Disk: add_message("first") [write V1]
StoreA->>StoreA: _load_session() → cache miss → reads V1
StoreB->>Disk: add_message("second") [write V2]
Caller->>StoreA: get_chat_history()
StoreA->>StoreA: _load_session() → cache hit → returns V1 (stale!)
Note over StoreA,StoreB: After fix — always fresh
StoreB->>Disk: add_message("first") [write V1]
StoreA->>StoreA: _load_session() → cache primed with V1 (test setup)
StoreB->>Disk: add_message("second") [write V2]
Caller->>StoreA: get_chat_history()
StoreA->>FileLock: acquire(session.json)
FileLock-->>StoreA: locked
StoreA->>Disk: _load_session_from_disk() → reads V2
StoreA->>FileLock: release
StoreA->>StoreA: update cache with V2
StoreA-->>Caller: returns V2 (fresh ✓)
Reviews (1): Last reviewed commit: "fix: address session store test failure ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _read_session_fresh(self, session_id: str) -> SessionData: | ||
| """Reload session from disk and refresh the in-process cache.""" | ||
| filepath = self._get_session_path(session_id) | ||
| with FileLock(filepath, self.lock_timeout): | ||
| session = self._load_session_from_disk(session_id, filepath) | ||
| with self._lock: | ||
| self._cache[session_id] = session | ||
| return session |
There was a problem hiding this comment.
Between releasing FileLock and acquiring self._lock, another thread or process can call _modify_session_locked, which writes a newer version to disk and updates the cache. When _read_session_fresh then acquires self._lock, it overwrites that newer cached value with the slightly older snapshot it read under the lock. Because all public read paths now always call _read_session_fresh, the next caller will re-read from disk and recover — but any code that still calls _load_session (including the tests that prime the cache) will observe the downgraded value until the next disk read.
| session_ids = self.list_sessions_by_agent(agent_name, limit) | ||
| return [self._load_session(sid) for sid in session_ids] | ||
| return [self._read_session_fresh(sid) for sid in session_ids] |
There was a problem hiding this comment.
get_sessions_by_agent
list_sessions_by_agent (called on line 657) already reads every JSON file in the directory once (without FileLock) to filter by agent_name. Then _read_session_fresh re-reads each matched session file a second time under FileLock. For an agent with many sessions this results in 2 × N full file reads in a tight loop, holding a separate lock for each. The list scan reads are also unprotected, so a write that happens between the two reads will cause list_sessions_by_agent and _read_session_fresh to disagree on the session content — the latter will always win, but the redundant scan adds latency for no correctness benefit.
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