From d6d6adc768ed84b271debdfda2114affe1297805 Mon Sep 17 00:00:00 2001 From: chauncygu Date: Sun, 10 May 2026 14:46:30 -0700 Subject: [PATCH] fix(web): pass user_id to remove_chat_session from the stale-session reaper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reaper thread spawned by web/server.py:_reap_stale_sessions called remove_chat_session(sid) without user_id, but remove_chat_session requires user_id (ownership check parity with the per-user DELETE endpoint). Every reaper tick raised: TypeError: remove_chat_session() missing 1 required positional argument: 'user_id' The daemon thread died on first invocation, so stale ChatSession objects accumulated in the in-memory cache instead of being evicted when their last_active aged past _IDLE_TIMEOUT — a slow memory leak in long-running web deployments. Fix: the ChatSession object already carries .user_id from its constructor; capture (sid, user_id) pairs under _chat_lock and apply remove_chat_session(sid, user_id) outside the lock (matches the existing collect-then-act pattern; avoids reentrant locking since remove_chat_session re-acquires the same lock to pop the cache). Regression test: test_reap_stale_chat_sessions_passes_user_id pins the call site — creates a session, rewinds last_active so is_stale() fires, runs the reaper, and asserts (a) no TypeError and (b) the session is actually evicted from _chat_sessions. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_web_api.py | 24 ++++++++++++++++++++++++ web/api.py | 16 +++++++++++----- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/tests/test_web_api.py b/tests/test_web_api.py index 6489535..1fc47f4 100644 --- a/tests/test_web_api.py +++ b/tests/test_web_api.py @@ -445,6 +445,30 @@ def test_session_list_includes_folder_id(server_url): assert s["folder_id"] is None +def test_reap_stale_chat_sessions_passes_user_id(server_url): + """Regression for the daemon-thread crash where the reaper called + remove_chat_session(sid) without user_id and the call raised + TypeError on every cycle. Pre-fix this caused stale sessions to + accumulate forever (the reaper thread died on first invocation).""" + with _client(server_url) as c: + _register(c, "alice") + sid = c.post("/api/prompt", + json={"prompt": "", "session_id": ""}).json()["session_id"] + # Force this session into the "stale + idle" state by rewinding + # its last_active far enough that is_stale() returns True. + from web import api as _apimod + sess = _apimod._chat_sessions.get(sid) + assert sess is not None, "session should be in the in-memory cache" + import time as _time + sess.last_active = _time.monotonic() - 1e9 # very old + # Reaper must run cleanly — pre-fix this raised + # TypeError: remove_chat_session() missing 1 required positional + # argument: 'user_id' + _apimod.reap_stale_chat_sessions() + # And it must actually evict the session + assert sid not in _apimod._chat_sessions + + def test_cross_user_isolation(server_url): with _client(server_url) as ca: _register(ca, "alice") diff --git a/web/api.py b/web/api.py index 4fd042f..9025f37 100644 --- a/web/api.py +++ b/web/api.py @@ -1133,11 +1133,17 @@ def get_available_models() -> list[dict]: def reap_stale_chat_sessions(): - """Called periodically by server.py's reaper thread.""" - stale: list[str] = [] + """Called periodically by server.py's reaper thread. + + `remove_chat_session` requires the owning user_id for ownership-check + parity with the per-user DELETE endpoint, so we capture it from the + cached ChatSession object — collecting `(sid, user_id)` pairs under the + lock and applying outside it (remove_chat_session re-acquires). + """ + stale: list[tuple[str, int]] = [] with _chat_lock: for sid, session in _chat_sessions.items(): if session.is_stale() and session.is_idle(): - stale.append(sid) - for sid in stale: - remove_chat_session(sid) + stale.append((sid, session.user_id)) + for sid, user_id in stale: + remove_chat_session(sid, user_id)