mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-06-15 17:25:26 -04:00
Joshua Valderrama
35b4dd2824
* docs: add implementation plan for fixing chat context drifting (#135) * fix: make Session.history immutable + fix {}.history crash - Session.history now exposes a COPY of the internal _history list - add_message() replaces history with a fresh copy each time - get_context_messages() derives from _history directly - replace_messages() updates both _history and history - truncate_messages() updates both _history and history - _persist_message() line 207: fixed {}.history fallback crash - Added 11 tests for session isolation and edge cases Addresses #135 root cause #1: shared mutable references * fix: task scheduler uses SessionManager methods instead of overwriting sessions - Added ensure_task_session() to SessionManager (checks cache first) - Task scheduler now uses ensure_task_session() instead of direct dict assignment - Task scheduler now uses SessionManager.add_message() for message persistence - Removed direct sess_obj.history.append() that was silently losing data Addresses #135 root causes #2 and #3 * fix: add age guard to cleanup_empty_sessions — don't delete sessions <1h old Prevents the cleanup task from deleting sessions that were just created and haven't received any messages yet (message_count == 0). Addresses #135 root cause #5 * test: comprehensive session isolation tests (10/10 passing) * refactor: consolidate _session_manager into singleton pattern - Added set_session_manager_instance / get_session_manager_instance to core/models - kept backward-compat aliases (set_session_manager, get_session_manager) - session_manager.py re-exports the singleton functions - ai_interaction.set_session_manager now syncs with the core singleton - context_compactor uses get_session_manager_instance() instead of getattr hack - app.py initializes the singleton once Addresses #135 root cause #4: fragile global wiring * test: add concurrent session isolation integration tests Verifies: - Concurrent add_message to different sessions doesn't cross-contaminate - Rapid parallel writes maintain isolation - Read-write concurrent access is safe All 3 async tests pass, proving the immutable history fix works under concurrency * fix: pre-import core.models in conftest to prevent test pollution test_agent_loop.py stubs sys.modules['core.models'] = MagicMock() at module level during collection. Any test collected after it imports Session as a MagicMock. Pre-importing core.models in conftest.py before test_agent_loop.py's module-level code runs prevents this. * fix: make .history authoritative mutable list, address PR review Per review feedback: keep .history as the authoritative mutable list so existing code doing .history.pop(), .history = [...], etc. still works. Fix the cross-contamination bug by ensuring __post_init__() gives each Session its OWN unique history list (never shared). Changes: - core/models.py: .history IS the authoritative list. _history aliases it. Each Session gets its own list in __post_init__. - core/session_manager.py: add_message() delegates to Session.add_message() instead of appending directly — no double-append, single source of truth. - tests/test_session_manager.py: updated test to reflect that .history references see new messages (same list, not a snapshot). - docs/plans/2026-06-01-fix-chat-context-drifting.md: removed (not for shipping — useful design context but too much process/doc to ship). All 272 tests pass (3 pre-existing failures unrelated). * Fix session manager message persistence * Fix session history alias regressions * Fix session history aliasing and task delivery
113 lines
3.8 KiB
Python
113 lines
3.8 KiB
Python
"""Integration tests: concurrent chat sessions must not leak.
|
|
|
|
These tests verify that the async streaming chat path maintains session
|
|
isolation even under concurrent access patterns.
|
|
"""
|
|
|
|
import asyncio
|
|
import sys
|
|
import os
|
|
sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
|
|
|
|
import pytest
|
|
|
|
from core.models import Session, ChatMessage
|
|
from core.session_manager import SessionManager
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_concurrent_sessions_have_independent_history():
|
|
"""Simulating concurrent message adds to different sessions."""
|
|
sm = SessionManager()
|
|
sm.sessions = {} # Bypass DB load
|
|
|
|
s1 = Session(id="sess-a", name="Chat A", endpoint_url="http://ep", model="model-a")
|
|
s2 = Session(id="sess-b", name="Chat B", endpoint_url="http://ep", model="model-b")
|
|
sm.sessions["sess-a"] = s1
|
|
sm.sessions["sess-b"] = s2
|
|
|
|
async def add_to_session(sid, msgs):
|
|
sess = sm.sessions[sid]
|
|
for role, content in msgs:
|
|
sess.add_message(ChatMessage(role, content))
|
|
|
|
# Simulate concurrent adds
|
|
await asyncio.gather(
|
|
add_to_session("sess-a", [("user", "hello from A"), ("assistant", "reply A")]),
|
|
add_to_session("sess-b", [("user", "hello from B")]),
|
|
)
|
|
|
|
a = sm.sessions["sess-a"]
|
|
b = sm.sessions["sess-b"]
|
|
|
|
assert len(a.history) == 2, f"Session A has {len(a.history)} messages, expected 2"
|
|
assert len(b.history) == 1, f"Session B has {len(b.history)} messages, expected 1"
|
|
assert b.history[0].content == "hello from B"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_concurrent_add_message_does_not_cross_contaminate():
|
|
"""Concurrent add_message calls must not write to each other's sessions."""
|
|
sm = SessionManager()
|
|
sm.sessions = {}
|
|
|
|
s1 = Session(id="a", name="A", endpoint_url="http://ep", model="m1")
|
|
s2 = Session(id="b", name="B", endpoint_url="http://ep", model="m2")
|
|
sm.sessions["a"] = s1
|
|
sm.sessions["b"] = s2
|
|
|
|
async def rapid_add(sid, count):
|
|
sess = sm.sessions[sid]
|
|
for i in range(count):
|
|
sess.add_message(ChatMessage("user", f"msg_{i}_from_{sid}"))
|
|
|
|
await asyncio.gather(
|
|
rapid_add("a", 5),
|
|
rapid_add("b", 5),
|
|
rapid_add("a", 3), # More adds to A
|
|
)
|
|
|
|
a = sm.sessions["a"]
|
|
b = sm.sessions["b"]
|
|
|
|
assert len(a.history) == 8, f"Session A has {len(a.history)} messages"
|
|
assert len(b.history) == 5, f"Session B has {len(b.history)} messages"
|
|
# Verify B's messages are purely from B
|
|
for msg in b.history:
|
|
assert msg.content.endswith("_from_b"), f"Session B has cross-contaminated: {msg.content}"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_concurrent_read_write_isolation():
|
|
"""Reading one session while writing to another must return correct data."""
|
|
sm = SessionManager()
|
|
sm.sessions = {}
|
|
|
|
s1 = Session(id="reader", name="Reader", endpoint_url="http://ep", model="m")
|
|
s2 = Session(id="writer", name="Writer", endpoint_url="http://ep", model="m")
|
|
sm.sessions["reader"] = s1
|
|
sm.sessions["writer"] = s2
|
|
|
|
# Pre-populate reader
|
|
s1.add_message(ChatMessage("user", "original"))
|
|
|
|
async def read_and_check():
|
|
for _ in range(20):
|
|
sess = sm.sessions["reader"]
|
|
hist = sess.get_context_messages()
|
|
# Should never see writer's messages
|
|
for msg in hist:
|
|
assert "writer_data" not in msg.get("content", ""), "Reader saw writer data!"
|
|
|
|
async def write_to_writer():
|
|
for i in range(20):
|
|
sm.sessions["writer"].add_message(ChatMessage("user", f"writer_data_{i}"))
|
|
|
|
await asyncio.gather(read_and_check(), write_to_writer())
|
|
|
|
# Final state check
|
|
reader = sm.sessions["reader"]
|
|
writer = sm.sessions["writer"]
|
|
assert len(reader.history) == 1, "Reader history mutated!"
|
|
assert len(writer.history) == 20, f"Writer has {len(writer.history)} messages"
|