Fix unbounded chat history leading to OOM (fixes #447) - #492
Fix unbounded chat history leading to OOM (fixes #447)#492atul-upadhyay-7 wants to merge 2 commits into
Conversation
|
@atul-upadhyay-7 is attempting to deploy a commit to the firefistisdead's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds a configurable ChangesBounded Chat History
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
github-actions
Bot
added
bug
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a configurable cap on per-session chat history and enforces that cap when loading, appending, and persisting sessions to prevent unbounded growth.
Changes:
- Introduce
MAX_CHAT_HISTORY_SIZEenv var and document it in.env.example - Truncate chat history to
MAX_CHAT_HISTORY_SIZEexchanges during session load/save/snapshot and onappend_chat_exchange - Add a unit test for truncation behavior; disable an existing PDF worker test
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| rag-service/main.py | Enforces chat history truncation across load/save/snapshot and when appending new exchanges; adds MAX_CHAT_HISTORY_SIZE config |
| rag-service/test_main.py | Adds truncation test; comments out PDF worker test and removes worker import |
| .env.example | Documents MAX_CHAT_HISTORY_SIZE env var |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (2)
rag-service/main.py (1)
183-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated truncation expression into a helper.
The
chat[-(MAX_CHAT_HISTORY_SIZE * 2):]pattern (and the associated length check) is duplicated across five sites (load, overlay, save, append, snapshot). Any future change to the truncation policy (e.g., switching from*2to counting user/bot exchanges explicitly) requires editing all five in lockstep, and it's easy to miss one.♻️ Proposed helper
+def _cap_chat_history(chat: list) -> list: + """Return chat trimmed to the most recent MAX_CHAT_HISTORY_SIZE exchanges.""" + max_entries = MAX_CHAT_HISTORY_SIZE * 2 + return chat[-max_entries:] if len(chat) > max_entries else chatThen replace each occurrence, e.g.:
- chat_history = normalize_chat_history(meta.get("chat", [])) - if len(chat_history) > MAX_CHAT_HISTORY_SIZE * 2: - chat_history = chat_history[-(MAX_CHAT_HISTORY_SIZE * 2):] - meta["chat"] = chat_history + meta["chat"] = _cap_chat_history(normalize_chat_history(meta.get("chat", [])))Also applies to: 203-206, 236-236, 370-373, 1576-1576
🤖 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 183 - 186, The chat truncation logic is duplicated in several places, so extract the repeated length check and slice based on MAX_CHAT_HISTORY_SIZE into a single helper and use it everywhere chat history is normalized or persisted. Update the related code paths in the functions handling load, overlay, save, append, and snapshot so they all call the same helper instead of repeating len(chat_history) > MAX_CHAT_HISTORY_SIZE * 2 and chat_history[-(MAX_CHAT_HISTORY_SIZE * 2):].rag-service/test_main.py (1)
192-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out test instead of leaving it disabled in code.
This test skeleton is fully commented out and unrelated to the chat-history truncation change in this cohort. Dead/commented-out test code accumulates confusion over time — either finish and enable it (using
pytest.mark.skip(reason=...)if intentionally deferred) or drop it from this PR.♻️ Suggested cleanup
-# def test_extract_pdf_text_worker_enforces_page_limit(tmp_path): -# import fitz -# -# pdf_path = tmp_path / "hello.pdf" -# doc = fitz.open() -# doc.new_page(width=300, height=144) -# doc.save(str(pdf_path)) -# doc.close() -# -# # Use a local queue and call the worker directly (no subprocess) to validate limit logic. -# q = multiprocessing.Queue(maxsize=1) -# _extract_pdf_text_worker(str(pdf_path), max_pages=0, max_chars=1000, out_queue=q) -# result = q.get(timeout=2) -# assert result["ok"] is False -# assert "too many pages" in result["error"].lower()🤖 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/test_main.py` around lines 192 - 209, The commented-out test in test_main.py should not remain as dead code; either restore it as an active test or remove it from this PR if it is not part of the chat-history truncation change. Update the relevant test block around test_extract_pdf_text_worker_enforces_page_limit so it is either implemented and runnable, or deleted entirely, and use pytest.mark.skip only if you intentionally want to keep it deferred.
🤖 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`:
- Line 793: The MAX_CHAT_HISTORY_SIZE setting is used by the chat truncation
logic in main.py, but values of 0 or negative currently bypass or corrupt
trimming instead of being rejected or clamped. Add startup validation where
MAX_CHAT_HISTORY_SIZE is read so the app either enforces a minimum positive
value or falls back to a safe default, and make sure the history-slicing code
paths that rely on this constant use the validated value consistently in the
chat history handling functions.
- Line 793: Move the MAX_CHAT_HISTORY_SIZE assignment so it is defined before
load_sessions() is called at import time, since load_sessions() currently
references that symbol before it exists and can trigger a NameError on startup.
Update the module-level initialization order in main.py so sessions =
load_sessions() happens only after MAX_CHAT_HISTORY_SIZE is set, keeping the
existing load_sessions() and sessions setup intact.
---
Nitpick comments:
In `@rag-service/main.py`:
- Around line 183-186: The chat truncation logic is duplicated in several
places, so extract the repeated length check and slice based on
MAX_CHAT_HISTORY_SIZE into a single helper and use it everywhere chat history is
normalized or persisted. Update the related code paths in the functions handling
load, overlay, save, append, and snapshot so they all call the same helper
instead of repeating len(chat_history) > MAX_CHAT_HISTORY_SIZE * 2 and
chat_history[-(MAX_CHAT_HISTORY_SIZE * 2):].
In `@rag-service/test_main.py`:
- Around line 192-209: The commented-out test in test_main.py should not remain
as dead code; either restore it as an active test or remove it from this PR if
it is not part of the chat-history truncation change. Update the relevant test
block around test_extract_pdf_text_worker_enforces_page_limit so it is either
implemented and runnable, or deleted entirely, and use pytest.mark.skip only if
you intentionally want to keep it deferred.
🪄 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 Plus
Run ID: 702ceb64-bef7-4624-861d-369fa916d785
📒 Files selected for processing (3)
.env.examplerag-service/main.pyrag-service/test_main.py
| # Configurable session TTL and max cap | ||
| SESSION_TTL_MINUTES = int(os.getenv("SESSION_TTL_MINUTES", "43200")) # 30 days default for persistence | ||
| MAX_ACTIVE_SESSIONS = int(os.getenv("MAX_ACTIVE_SESSIONS", "1000")) | ||
| MAX_CHAT_HISTORY_SIZE = int(os.getenv("MAX_CHAT_HISTORY_SIZE", "100")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
MAX_CHAT_HISTORY_SIZE=0 (or negative) silently disables truncation instead of capping history.
Slicing with [-(0*2):] is list[0:] (i.e. the full list), and a negative MAX_CHAT_HISTORY_SIZE produces chat[positive_index:], dropping the wrong elements rather than raising or clamping. If an operator sets MAX_CHAT_HISTORY_SIZE=0 intending to disable/minimize chat retention, this bug means truncation never happens — undermining the OOM protection this PR is meant to add. Validate the value once at startup.
🛡️ Proposed fix
-MAX_CHAT_HISTORY_SIZE = int(os.getenv("MAX_CHAT_HISTORY_SIZE", "100"))
+MAX_CHAT_HISTORY_SIZE = max(1, int(os.getenv("MAX_CHAT_HISTORY_SIZE", "100")))Also applies to: 184-186, 205-206, 236-236, 371-373, 1576-1576
🤖 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` at line 793, The MAX_CHAT_HISTORY_SIZE setting is used
by the chat truncation logic in main.py, but values of 0 or negative currently
bypass or corrupt trimming instead of being rejected or clamped. Add startup
validation where MAX_CHAT_HISTORY_SIZE is read so the app either enforces a
minimum positive value or falls back to a safe default, and make sure the
history-slicing code paths that rely on this constant use the validated value
consistently in the chat history handling functions.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where load_sessions() is invoked at module scope relative to line 793
rg -n 'load_sessions\(\)' rag-service/main.py
rg -n '^MAX_CHAT_HISTORY_SIZE' rag-service/main.pyRepository: FireFistisDead/pdf-qa-bot
Length of output: 294
🏁 Script executed:
sed -n '173,214p' rag-service/main.py
printf '\n---\n'
sed -n '240,260p' rag-service/main.py
printf '\n---\n'
sed -n '785,800p' rag-service/main.pyRepository: FireFistisDead/pdf-qa-bot
Length of output: 4137
Move MAX_CHAT_HISTORY_SIZE above load_sessions() sessions = load_sessions() runs at import time, but load_sessions() reads MAX_CHAT_HISTORY_SIZE before it is assigned, which will raise NameError on startup.
🤖 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` at line 793, Move the MAX_CHAT_HISTORY_SIZE assignment
so it is defined before load_sessions() is called at import time, since
load_sessions() currently references that symbol before it exists and can
trigger a NameError on startup. Update the module-level initialization order in
main.py so sessions = load_sessions() happens only after MAX_CHAT_HISTORY_SIZE
is set, keeping the existing load_sessions() and sessions setup intact.
Closes #447
This PR adds a
MAX_CHAT_HISTORY_SIZEconfigurable limit to prevent out-of-memory crashes due to unbounded chat histories. The limit is enforced across the active session memory, background persistence, and on-startup loads to ensure robust and bounded memory usage.Summary by CodeRabbit
New Features
Bug Fixes