fix: prevent duplicate session messages from persist+auto_save and save_state#1897
fix: prevent duplicate session messages from persist+auto_save and save_state#1897cursor[bot] wants to merge 2 commits into
Conversation
…ve_state When memory=history and auto_save share a session_id, per-turn _persist_message wrote messages that _auto_save_session then re-appended because _auto_save_last_index was not updated. Session.save_state() also re-appended the full chat history on every call instead of replacing it atomically. - Sync _auto_save_last_index in _persist_message when sessions match - Use set_chat_history() in Session._save_agent_chat_histories for idempotent saves - Add regression tests for both duplicate paths 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:
📝 WalkthroughWalkthroughSession and agent memory persistence are refactored to prevent duplicate message entries: Session._save_agent_chat_histories() now atomically replaces history via set_chat_history when available, and MemoryMixin._persist_message synchronizes the auto-save index. Tests verify no duplicates occur on repeated saves. ChangesChat History Duplicate Prevention
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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.
🧹 Nitpick comments (2)
src/praisonai-agents/praisonaiagents/agent/memory_mixin.py (1)
406-410: 💤 Low valuePotential race condition on
_auto_save_last_indexupdate.Lines 408-410 perform a read-modify-write on
_auto_save_last_indexwithout acquiring_history_lock. If multiple threads call_persist_messageconcurrently, the increment can be lost (both threads read the same value and write the same incremented value), causing_auto_save_sessionto re-save already-persisted messages on the next call.Typical agent usage is single-threaded (one turn at a time), so this race is unlikely in practice. However, for consistency with other
MemoryMixinmethods that protect shared state (e.g.,_add_to_chat_historyat line 81,_get_chat_history_lengthat line 109), consider wrapping the index update inwith self._history_lock:or document that_persist_messagemust be called serially.🤖 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/agent/memory_mixin.py` around lines 406 - 410, The read-modify-write of _auto_save_last_index in _persist_message can race; wrap the increment and getattr of self._auto_save_last_index inside the existing _history_lock to make the update atomic (e.g., inside with self._history_lock: read current value via getattr, increment, and assign), and keep the surrounding conditional that checks self.auto_save and self._session_id; alternatively, if you prefer not to change concurrency behavior, add a comment on _persist_message documenting that calls must be serialized and referencing _auto_save_last_index and _auto_save_session.src/praisonai-agents/praisonaiagents/session/api.py (1)
353-361: 💤 Low valueFallback path preserves duplicate-append behavior; empty messages silently skipped.
Two edge cases to consider:
Lines 355-361: When
set_chat_historyis unavailable, the fallback iterates and callsadd_messagefor every message on eachsave_state()invocation. This preserves the original duplicate-append bug for backends without atomic replacement. If this is intentional for backward compatibility, consider documenting it with a comment or logging a warning on first use.Line 353: When
messagesis empty (e.g.,chat_historycontains only non-dict entries), both the atomic and fallback paths are skipped silently. If this is expected, no action needed; otherwise, consider logging a debug message to help diagnose missing persistence.🤖 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/api.py` around lines 353 - 361, The fallback path currently appends each message via session_store.add_message and so re-introduces duplicate-append bugs for backends that lack session_store.set_chat_history; additionally an empty messages list is silently ignored. Change save_state() so that when messages is falsy you emit a debug log (or no-op with a comment) to make missing persistence visible, and when session_store lacks set_chat_history attempt an atomic replace: if the store exposes a clear/delete method (e.g. session_store.clear_chat_history or session_store.delete_chat_history) call that first for session_id, otherwise read existing history and replace/deduplicate before looping, and if neither is possible emit a one-time warning indicating the backend will append (so callers know to migrate) — reference session_store.set_chat_history, session_store.add_message, session_id, messages, and save_state().
🤖 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.
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py`:
- Around line 406-410: The read-modify-write of _auto_save_last_index in
_persist_message can race; wrap the increment and getattr of
self._auto_save_last_index inside the existing _history_lock to make the update
atomic (e.g., inside with self._history_lock: read current value via getattr,
increment, and assign), and keep the surrounding conditional that checks
self.auto_save and self._session_id; alternatively, if you prefer not to change
concurrency behavior, add a comment on _persist_message documenting that calls
must be serialized and referencing _auto_save_last_index and _auto_save_session.
In `@src/praisonai-agents/praisonaiagents/session/api.py`:
- Around line 353-361: The fallback path currently appends each message via
session_store.add_message and so re-introduces duplicate-append bugs for
backends that lack session_store.set_chat_history; additionally an empty
messages list is silently ignored. Change save_state() so that when messages is
falsy you emit a debug log (or no-op with a comment) to make missing persistence
visible, and when session_store lacks set_chat_history attempt an atomic
replace: if the store exposes a clear/delete method (e.g.
session_store.clear_chat_history or session_store.delete_chat_history) call that
first for session_id, otherwise read existing history and replace/deduplicate
before looping, and if neither is possible emit a one-time warning indicating
the backend will append (so callers know to migrate) — reference
session_store.set_chat_history, session_store.add_message, session_id, messages,
and save_state().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6c44b4e-bf46-4a81-9c3d-5661cb8ca887
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/agent/memory_mixin.pysrc/praisonai-agents/praisonaiagents/session/api.pysrc/praisonai-agents/tests/unit/test_memory_history_separation.py
|
@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: Fix Valid Issues
Phase 3: Final Verdict
Architecture Assessment ✅Protocol-driven design: ✅ EXCELLENT
Backward compatibility: ✅ MAINTAINED
Performance: ✅ IMPROVED
Issues Fixed ✅🔧 Thread Safety Issue (
🔧 Fallback Logging (
Core Fix Quality Assessment ✅✅ Root Cause Addressed: Both duplicate sources eliminated
✅ Test Coverage: Comprehensive regression tests added
✅ Minimal Impact: Surgical changes, no scope creep
Final Recommendation: APPROVE ✅This PR successfully fixes critical duplicate message bugs with excellent engineering:
The additional fixes I applied address the valid thread safety and observability concerns raised by CodeRabbit, making this PR even more robust. Commit pushed to PR: Files modified in review fixes:
|
- Wrap _auto_save_last_index increment in _history_lock for thread safety - Add warning when session store lacks set_chat_history (duplicate risk) - Add debug logging for empty message lists Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
2 participants
Bug and impact
Two related session persistence bugs caused duplicate chat messages to be written to disk:
memory="history"+auto_save: Per-turn_persist_message()saved each message, then_auto_save_session()re-appended the full history because_auto_save_last_indexwas never updated by the per-turn path. A single agent turn produced 4 messages instead of 2.Session.save_state():_save_agent_chat_histories()calledadd_message()for every message on each save, so repeatedsave_state()calls doubled/tripled history.Impact: Corrupted session files, bloated context windows, incorrect history on restore, and model confusion from duplicate turns.
Root cause
_persist_messageand_auto_save_sessionboth write to the same session ID but only_auto_save_sessiontracked the last-saved index._save_agent_chat_historiesused append semantics (add_message) instead of replace semantics for full-history saves.Fix
_auto_save_last_indexin_persist_messagewhenauto_save == _session_id.set_chat_history()inSession._save_agent_chat_historiesfor idempotent saves.Validation
test_memory_history_separation.pySummary by CodeRabbit
Release Notes