fix(rag): restore chat history and flashcards during session recovery#568
Conversation
|
@Sandeep6135 is attempting to deploy a commit to the firefistisdead's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
bug
📝 WalkthroughWalkthroughIn ChangesSession Recovery Data Restoration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Fixes a data-loss bug in the RAG service’s session recovery path by restoring per-session chat history and flashcards from on-disk metadata before the session is re-hydrated into memory, preventing the background flush thread from overwriting history with empty lists.
Changes:
- Load
<session_dir>/session_meta.jsonduring_recover_session_unlockedto restorechatandflashcards. - Normalize recovered
chatvianormalize_chat_historybefore storing it in the recovered session metadata. - Include restored
chat/flashcardsin the recovered in-memorymetadict.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| chat = [] | ||
| flashcards = [] | ||
| meta_path = os.path.join(session_dir, "session_meta.json") | ||
| if os.path.isfile(meta_path): | ||
| try: | ||
| with open(meta_path, "r", encoding="utf-8") as f: | ||
| per = json.load(f) | ||
| if isinstance(per.get("chat"), list): | ||
| chat = normalize_chat_history(per["chat"]) | ||
| if isinstance(per.get("flashcards"), list): | ||
| flashcards = per["flashcards"] | ||
| except Exception as e: | ||
| logger.warning("Failed to load session metadata during recovery for %s: %s", session_id, e) |
| chat = [] | ||
| flashcards = [] | ||
| meta_path = os.path.join(session_dir, "session_meta.json") |
![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 `@rag-service/main.py`:
- Around line 1258-1271: The code initializes chat and flashcards as empty lists
and silently proceeds with those defaults if the metadata file exists but cannot
be read or parsed, which can cause data loss. To fix this fail-open
vulnerability, remove the default empty list initialization and instead only set
chat and flashcards if the metadata loads successfully. Wrap the entire metadata
loading logic (from opening the file through the isinstance checks) in a
try-except block that catches only specific expected exceptions like
JSONDecodeError and IOError rather than the broad Exception catch, and when any
of these expected exceptions occur and the file exists, return None or abort the
recovery process instead of proceeding with empty defaults. This ensures that
transient issues reading session_meta.json do not result in empty data being
persisted and overwriting existing chat/flashcard history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| chat = [] | ||
| flashcards = [] | ||
| meta_path = os.path.join(session_dir, "session_meta.json") | ||
| if os.path.isfile(meta_path): | ||
| try: | ||
| with open(meta_path, "r", encoding="utf-8") as f: | ||
| per = json.load(f) | ||
| if isinstance(per.get("chat"), list): | ||
| chat = normalize_chat_history(per["chat"]) | ||
| if isinstance(per.get("flashcards"), list): | ||
| flashcards = per["flashcards"] | ||
| except Exception as e: | ||
| logger.warning("Failed to load session metadata during recovery for %s: %s", session_id, e) | ||
|
|
There was a problem hiding this comment.
Fail-open recovery still permits chat/flashcard data loss on metadata read errors.
On Line 1258 and Line 1259, defaults are []; on Line 1269, any exception during read/parse/normalize is swallowed, and recovery proceeds with those empty lists. That means transient I/O/JSON issues can still lead to empty chat/flashcards being persisted later, recreating the overwrite-loss path this PR is fixing.
Prefer fail-closed when session_meta.json exists but cannot be safely loaded (e.g., abort recovery/return None), and narrow caught exceptions to expected read/parse failures.
Suggested patch
- chat = []
- flashcards = []
+ chat = []
+ flashcards = []
meta_path = os.path.join(session_dir, "session_meta.json")
if os.path.isfile(meta_path):
try:
with open(meta_path, "r", encoding="utf-8") as f:
per = json.load(f)
if isinstance(per.get("chat"), list):
chat = normalize_chat_history(per["chat"])
if isinstance(per.get("flashcards"), list):
flashcards = per["flashcards"]
- except Exception as e:
+ except (OSError, json.JSONDecodeError, UnicodeDecodeError) as e:
logger.warning("Failed to load session metadata during recovery for %s: %s", session_id, e)
+ return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chat = [] | |
| flashcards = [] | |
| meta_path = os.path.join(session_dir, "session_meta.json") | |
| if os.path.isfile(meta_path): | |
| try: | |
| with open(meta_path, "r", encoding="utf-8") as f: | |
| per = json.load(f) | |
| if isinstance(per.get("chat"), list): | |
| chat = normalize_chat_history(per["chat"]) | |
| if isinstance(per.get("flashcards"), list): | |
| flashcards = per["flashcards"] | |
| except Exception as e: | |
| logger.warning("Failed to load session metadata during recovery for %s: %s", session_id, e) | |
| chat = [] | |
| flashcards = [] | |
| meta_path = os.path.join(session_dir, "session_meta.json") | |
| if os.path.isfile(meta_path): | |
| try: | |
| with open(meta_path, "r", encoding="utf-8") as f: | |
| per = json.load(f) | |
| if isinstance(per.get("chat"), list): | |
| chat = normalize_chat_history(per["chat"]) | |
| if isinstance(per.get("flashcards"), list): | |
| flashcards = per["flashcards"] | |
| except (OSError, json.JSONDecodeError, UnicodeDecodeError) as e: | |
| logger.warning("Failed to load session metadata during recovery for %s: %s", session_id, e) | |
| return None |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 1269-1269: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@rag-service/main.py` around lines 1258 - 1271, The code initializes chat and
flashcards as empty lists and silently proceeds with those defaults if the
metadata file exists but cannot be read or parsed, which can cause data loss. To
fix this fail-open vulnerability, remove the default empty list initialization
and instead only set chat and flashcards if the metadata loads successfully.
Wrap the entire metadata loading logic (from opening the file through the
isinstance checks) in a try-except block that catches only specific expected
exceptions like JSONDecodeError and IOError rather than the broad Exception
catch, and when any of these expected exceptions occur and the file exists,
return None or abort the recovery process instead of proceeding with empty
defaults. This ensures that transient issues reading session_meta.json do not
result in empty data being persisted and overwriting existing chat/flashcard
history.
Source: Linters/SAST tools
Pull Request: Resolve Critical Data Loss inside Session Recovery Flow
Closes #561
📌 Classification & Priority
bug-fixcriticalrag-serviceexceptional-data-protection📖 Summary
Important
This PR resolves a critical logical defect in the session recovery flow where a user's entire chat history and flashcards were permanently deleted from disk when an evicted session or restarted session was recovered from the registry database.
🔴 Problem
When an inactive session was recovered using
_recover_session_unlocked, it restored metadata values from the general session registry but omitted loading the"chat"and"flashcards"fields. Consequently, the recovered in-memory session object lacked these keys. When the background thread next ran_snapshot_session_for_persistence, it resolved the missing fields to empty lists ([]), overwrotesession_meta.jsonon disk, and wiped out the user's historical data.🟢 Solution
Updated the session recovery function to:
<session_dir>/session_meta.jsoninside the recovered session directory.chatandflashcardsstates.normalize_chat_history.🧪 Steps to Reproduce
chatandflashcardshistory./validate-session-writeor/askto recover the session from registry.session_meta.jsonin the database directory.🔍 Expected Behaviour
The session recovery routine retrieves the existing
session_meta.jsonfile on disk and fully restores the user's chat history and flashcards.❌ Actual Behaviour (Before Fix)
The chat log and flashcards were missing during the active session after recovery, and the corresponding lists were overwritten with empty arrays (
[]) in the JSON file on disk, permanently wiping out user data.🛠️ Code Diff Walkthrough
rag-service/main.py