fix: session message loss in managed stream and bot history saves#1768
fix: session message loss in managed stream and bot history saves#1768cursor[bot] wants to merge 2 commits into
Conversation
- Persist user/assistant messages and state after LocalManagedAgent.stream() - Add DefaultSessionStore.set_chat_history for atomic locked replace - BotSessionManager uses set_chat_history instead of clear + re-add loop - DbSessionAdapter.update_session_metadata accepts **fields like file store - Add regression tests for stream persistence and metadata API 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 atomic session message persistence to prevent concurrent message loss in managed agents. It introduces ChangesSession history persistence in managed agents
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
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: 1
🤖 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`:
- Around line 534-553: set_chat_history currently rebuilds messages via
SessionMessage(...) inside the nested _apply function but drops any provided
timestamp because SessionMessage uses time.time() by default; update the
SessionMessage construction in _apply to pass timestamp=msg.get("timestamp",
time.time()) (or equivalent) so existing timestamps from the incoming messages
are preserved when present (ensure the value is a numeric timestamp if
necessary) — change the SessionMessage instantiation in set_chat_history/_apply
accordingly.
🪄 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: 0a172718-0a5d-4c65-bbd4-9b115d91d1c2
📒 Files selected for processing (7)
src/praisonai-agents/praisonaiagents/session/store.pysrc/praisonai-agents/tests/managed/test_managed_persistence.pysrc/praisonai-agents/tests/unit/session/test_session_store.pysrc/praisonai/praisonai/bots/_session.pysrc/praisonai/praisonai/integrations/db_session_adapter.pysrc/praisonai/praisonai/integrations/managed_local.pysrc/praisonai/tests/unit/integrations/test_managed_agents.py
| def set_chat_history( | ||
| self, | ||
| session_id: str, | ||
| messages: List[Dict[str, str]], | ||
| ) -> bool: | ||
| """Replace session messages atomically (file-locked read-modify-write).""" | ||
|
|
||
| def _apply(session: SessionData) -> None: | ||
| session.messages.clear() | ||
| for msg in messages: | ||
| session.messages.append( | ||
| SessionMessage( | ||
| role=msg.get("role", "user"), | ||
| content=msg.get("content", ""), | ||
| ) | ||
| ) | ||
|
|
||
| return self._modify_session_locked( | ||
| session_id, _apply, error_label="set chat history" | ||
| ) |
There was a problem hiding this comment.
Timestamp preservation in message replacement.
The set_chat_history method creates new SessionMessage objects without preserving original timestamps from the input dicts. When the SessionMessage constructor is called (lines 545-549), it uses the default timestamp=time.time(), even if the input msg dict contains a timestamp key.
This means calling set_chat_history with messages that have existing timestamps will lose that temporal information, which could affect history ordering or analytics that depend on accurate message timing.
⏰ Proposed fix to preserve timestamps
def _apply(session: SessionData) -> None:
session.messages.clear()
for msg in messages:
session.messages.append(
SessionMessage(
role=msg.get("role", "user"),
content=msg.get("content", ""),
+ timestamp=msg.get("timestamp", time.time()),
+ metadata=msg.get("metadata", {}),
)
)🤖 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` around lines 534 -
553, set_chat_history currently rebuilds messages via SessionMessage(...) inside
the nested _apply function but drops any provided timestamp because
SessionMessage uses time.time() by default; update the SessionMessage
construction in _apply to pass timestamp=msg.get("timestamp", time.time()) (or
equivalent) so existing timestamps from the incoming messages are preserved when
present (ensure the value is a numeric timestamp if necessary) — change the
SessionMessage instantiation in set_chat_history/_apply accordingly.
|
@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 Architecture Review & Fix ImplementationPhase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
✅ Final Architectural ReviewProtocol-Driven Design ComplianceEXCELLENT - This PR perfectly follows AGENTS.md principles:
Backward CompatibilityPERFECT - Zero breaking changes:
Performance & SecurityNO REGRESSIONS:
Issues Fixed1. Timestamp Preservation Bug (Fixed in commit 8dbaba8)
2. Session Message Loss in Streaming
3. Atomic History Replacement
4. API Signature Mismatch
Test Coverage
Code Quality AssessmentEXCEPTIONAL:
🎯 Final Verdict: APPROVED ✅This PR exemplifies excellent PraisonAI architecture:
Ready for merge - all identified issues have been resolved. Modified Files:
Skipped: No other blocking issues found after comprehensive review. |
- Fix SessionMessage construction in set_chat_history to preserve original timestamp when provided in input messages - Also preserve metadata field for completeness - Prevents loss of temporal ordering information during history replacement Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
2 participants
Summary
Scheduled critical-bug scan found two session persistence gaps in recent session/managed-agent work (after #1724–#1737 fixes).
Bugs fixed
1.
LocalManagedAgent.stream()dropped session messagesexecute()persisted messages.async for chunk in managed.stream(prompt)with asession_storeor DB-backed adapter.execute()— persist user prompt, assistant full text, usage, and state in the stream workerfinallyblock.2.
BotSessionManagernon-atomic history rewriteclear_sessionand the subsequentadd_messageloop.DefaultSessionStore.set_chat_history()(single locked RMW) andBotSessionManageruses it when available.3.
DbSessionAdapter.update_session_metadataAPI mismatch (minor)**fields(caught inmemory_mixin).**fieldslikeDefaultSessionStore.Validation
test_local_managed_stream_persists_messagestest_set_chat_history_replaces_under_locktest_update_session_metadata_accepts_keyword_fieldstests/unit/session/test_session_store.py(35 passed)Summary by CodeRabbit
New Features
Bug Fixes
Tests