fix: prevent CLI session message loss on concurrent saves#1868
fix: prevent CLI session message loss on concurrent saves#1868cursor[bot] wants to merge 2 commits into
Conversation
UnifiedSessionStore.save() wrote the full in-memory session without reloading from disk under lock. When TUI and --interactive (or two processes) shared a session_id, the last writer could drop messages added by the other. Reload from disk under exclusive lock, merge messages by identity, and always load from disk when the session file exists. 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? |
✅ Action performedReview finished.
|
|
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:
📝 WalkthroughWalkthrough
ChangesConcurrent-safe session persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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: 2
🤖 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/praisonai/cli/session/unified.py`:
- Around line 151-158: The current merge in the save path (building
messages_by_key from disk.messages and setdefault-ing from incoming.messages
using _message_key) makes saves effectively append-only and silently reverts
destructive in-memory ops like UnifiedSession.clear_messages(); fix by tracking
a persisted baseline/version when loading a session and using it under the same
lock in save to decide how to merge: if the incoming snapshot's base/version
matches the persisted baseline, replace disk.messages with incoming.messages
(preserving incoming order) instead of unioning; if versions differ, perform a
proper three-way reconciliation against the persisted baseline (apply explicit
additions/edits/deletes from incoming relative to the baseline) using
_message_key to identify messages and then update the persisted baseline/version
after a successful write.
- Around line 160-163: The current update in unified.py uses max() to merge
absolute counters (disk.total_input_tokens, disk.total_output_tokens,
disk.total_cost, disk.request_count) which loses concurrent increments; change
update_stats() to record a per-session baseline/version on load (e.g.,
session.version or session.baseline_counters), compute and persist only the
delta under the exclusive lock when saving (read current stored totals, add the
delta from this instance, increment the version), and remove the max()-based
merging; also add a regression test that creates two store instances, both call
update_stats() with different increments then save concurrently to assert totals
are the sum of both deltas and versioning prevents lost updates.
🪄 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: e90c172e-cd92-47bb-9f30-40f6cccac9c2
📒 Files selected for processing (2)
src/praisonai/praisonai/cli/session/unified.pysrc/praisonai/tests/unit/cli/test_unified_session.py
| messages_by_key = {self._message_key(m): m for m in disk.messages} | ||
| for msg in incoming.messages: | ||
| messages_by_key.setdefault(self._message_key(msg), msg) | ||
|
|
||
| disk.messages = sorted( | ||
| messages_by_key.values(), | ||
| key=lambda m: m.get("timestamp", ""), | ||
| ) |
There was a problem hiding this comment.
Union-only message merging makes clear_messages() impossible to persist.
This merge starts from disk.messages and only adds missing entries from the incoming snapshot, so any destructive update in memory—clear_messages(), truncation, or editing/removing a message—gets silently undone on save. That breaks the existing UnifiedSession.clear_messages() contract and turns save() into append-only behavior.
Suggested direction
Track a persisted base/version for each loaded session and resolve against that base under the lock. Without that baseline, the store cannot distinguish “stale snapshot missing concurrent appends” from “caller intentionally removed messages”.
🤖 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/praisonai/cli/session/unified.py` around lines 151 - 158, The
current merge in the save path (building messages_by_key from disk.messages and
setdefault-ing from incoming.messages using _message_key) makes saves
effectively append-only and silently reverts destructive in-memory ops like
UnifiedSession.clear_messages(); fix by tracking a persisted baseline/version
when loading a session and using it under the same lock in save to decide how to
merge: if the incoming snapshot's base/version matches the persisted baseline,
replace disk.messages with incoming.messages (preserving incoming order) instead
of unioning; if versions differ, perform a proper three-way reconciliation
against the persisted baseline (apply explicit additions/edits/deletes from
incoming relative to the baseline) using _message_key to identify messages and
then update the persisted baseline/version after a successful write.
| disk.total_input_tokens = max(disk.total_input_tokens, incoming.total_input_tokens) | ||
| disk.total_output_tokens = max(disk.total_output_tokens, incoming.total_output_tokens) | ||
| disk.total_cost = max(disk.total_cost, incoming.total_cost) | ||
| disk.request_count = max(disk.request_count, incoming.request_count) |
There was a problem hiding this comment.
max() still drops concurrent token/cost increments.
If two processes load the same session and both call update_stats(), they each advance from the same base totals. Merging these fields with max() preserves only one branch’s increment, so total_input_tokens, total_output_tokens, total_cost, and request_count will undercount after the second save.
Suggested direction
Persist a per-session baseline/version on load/save and apply deltas under the exclusive lock instead of merging absolute totals with max(). Please add a regression test with two store instances that both call update_stats() before saving.
🤖 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/praisonai/cli/session/unified.py` around lines 160 - 163, The
current update in unified.py uses max() to merge absolute counters
(disk.total_input_tokens, disk.total_output_tokens, disk.total_cost,
disk.request_count) which loses concurrent increments; change update_stats() to
record a per-session baseline/version on load (e.g., session.version or
session.baseline_counters), compute and persist only the delta under the
exclusive lock when saving (read current stored totals, add the delta from this
instance, increment the version), and remove the max()-based merging; also add a
regression test that creates two store instances, both call update_stats() with
different increments then save concurrently to assert totals are the sum of both
deltas and versioning prevents lost updates.
|
@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 - COMPLETED ✅Todo List:
Phase 1: Architecture Review ✅ PASSEDProtocol-driven: Changes properly located in wrapper layer ( Phase 2: Fixed Valid Issues ✅ COMPLETEDIssue 1 - Append-only merge behavior (CodeRabbit lines 151-158): ✅ FIXED
Issue 2 - max() dropping concurrent increments (CodeRabbit lines 160-163): ✅ FIXED
Phase 3: Final Verdict ✅ APPROVEFiles Modified:
What was NOT touched:
Validation Results:
Architecture Assessment: The fixes are architecturally sound, follow PraisonAI patterns, maintain backward compatibility, and resolve the core concurrency issues without breaking existing functionality. 🎯 RECOMMENDATION: APPROVE & MERGE The PR successfully fixes the CLI session message loss issue with a robust versioning approach that prevents both message loss and counter update loss in concurrent scenarios. |
- Add session versioning (_version, _baseline_counters) for proper merge resolution - Fix append-only merge behavior that silently reverted destructive operations like clear_messages() - Replace max() with delta-based counter merging to prevent concurrent increment loss - Add regression tests for concurrent stats updates and clear_messages persistence - Implement proper three-way merge for version conflicts Addresses CodeRabbit feedback from PR review. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
2 participants
Bug and impact
UnifiedSessionStore.save()wrote the full in-memory session to disk without reloading under lock first. When two CLI processes (e.g. TUI and--interactive) shared the samesession_id, the last writer could silently drop messages added by the other — chat history data loss.Trigger scenario:
msg1and keeps it in cache.msg2, saves.msg3and saves from stale cache.msg1,msg3—msg2is lost.Root cause
Commit #1837 added cross-platform file locking to
UnifiedSessionStore, butsave()still performed a blind whole-object overwrite.load()also returned stale cache entries without checking disk.Fix
save().(role, content, timestamp)identity so concurrent appends are preserved.DefaultSessionStorepattern).Validation
msg2lost).test_concurrent_save_preserves_messagesregression test.tests/unit/cli/test_unified_session.pypass.Summary by CodeRabbit
Bug Fixes
Tests