diff --git a/CHANGELOG.md b/CHANGELOG.md index 936be3d48d9..79ae1dba9b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -357,6 +357,18 @@ workspace subtree) and never enumerate blocked system roots. (`api/routes.py`, `api/workspace.py`, `static/panels.js`, `static/style.css`) (partial for #616) +## [v0.50.230] — 2026-04-27 + +### Fixed +- **No disk write for empty sessions** — `new_session()` no longer eagerly writes an empty + JSON file to disk. The session lives in the in-memory `SESSIONS` dict only; the first disk + write happens at the natural "this is now a real session" moment (first user message via + `/api/chat/start`, or explicit `s.save()` in the btw/background-agent paths). Eliminates + orphan `sessions/*.json` files that accumulated on every page reload, New Conversation click, + or onboarding pass without sending a message. Crash-safety: if the process exits between + create and first message, the session is lost — since it had no messages, there is nothing + to lose. (`api/models.py`) (#1171 follow-up, #1184) + ## [v0.50.229] — 2026-04-27 ### Performance diff --git a/api/models.py b/api/models.py index a8aa015e93a..4a01db51342 100644 --- a/api/models.py +++ b/api/models.py @@ -484,7 +484,21 @@ def get_session(sid, metadata_only=False): raise KeyError(sid) def new_session(workspace=None, model=None, profile=None): - """Create a new in-memory session and persist it. + """Create a new in-memory session. + + The session lives in the SESSIONS dict only — no disk write happens until + the first message is appended (#1171 follow-up). This avoids the + "ghost Untitled session on disk" pile-up that occurred when users clicked + New Conversation, reloaded the page, or completed onboarding without ever + sending a message. Subsequent code paths that populate state immediately + (btw / background agent at api/routes.py) call ``s.save()`` themselves + after setting title/messages, and ``_handle_chat_start`` saves the + session as soon as the user actually sends a message — both are the + natural first-write moments for a real session. + + Crash-safety: if the process exits between session creation and first + message, the session is lost. Since it had no messages, there is + nothing to lose. *profile* — when supplied by the caller (e.g. from the request body sent by the active browser tab), it is used directly so that concurrent clients @@ -510,7 +524,6 @@ def new_session(workspace=None, model=None, profile=None): SESSIONS.move_to_end(s.session_id) while len(SESSIONS) > SESSIONS_MAX: SESSIONS.popitem(last=False) - s.save() return s def _hide_from_default_sidebar(session: dict) -> bool: diff --git a/tests/test_empty_session_no_disk_write.py b/tests/test_empty_session_no_disk_write.py new file mode 100644 index 00000000000..9d73475f9c3 --- /dev/null +++ b/tests/test_empty_session_no_disk_write.py @@ -0,0 +1,130 @@ +""" +Regression tests for the "no disk write for empty sessions" follow-up to #1171. + +Lifecycle contract: + 1. ``new_session()`` adds the session to the in-memory ``SESSIONS`` dict but + does NOT write a JSON file to disk. + 2. The first ``s.save()`` happens when the session has real state to persist + (a user message via ``/api/chat/start``, or a populated title/messages + for btw / background agents). + 3. ``get_session(sid)`` is unchanged: it checks ``SESSIONS`` first, so an + unsaved session is still findable by ID for the brief window between + create and first message. + 4. ``all_sessions()`` already filters Untitled + 0-message sessions (#1171), + so an unsaved in-memory session does not surface in the sidebar even + though it lives in the SESSIONS dict. + +Crash-safety: if the process exits between create and first message, the +session is lost. There were no messages to lose, so this is an explicit +trade-off documented in ``new_session``'s docstring. +""" +import json +import time + +import pytest + +import api.models as models +from api.models import ( + SESSIONS, + Session, + all_sessions, + get_session, + new_session, +) + + +@pytest.fixture(autouse=True) +def _isolate(tmp_path, monkeypatch): + """Redirect SESSION_DIR and SESSION_INDEX_FILE to a fresh tmp dir.""" + session_dir = tmp_path / "sessions" + session_dir.mkdir() + index_file = session_dir / "_index.json" + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", index_file) + SESSIONS.clear() + yield session_dir + SESSIONS.clear() + + +# ── 1. new_session does not write to disk ─────────────────────────────────── + + +def test_new_session_does_not_write_to_disk(_isolate): + s = new_session() + assert not s.path.exists(), ( + "new_session() must not eagerly persist an empty session — disk write " + "is deferred until the first message is appended (#1171 follow-up)" + ) + + +def test_new_session_lives_in_memory(_isolate): + s = new_session() + assert s.session_id in SESSIONS + assert SESSIONS[s.session_id] is s + + +def test_get_session_finds_unsaved_session_by_id(_isolate): + """The brief window between create and first message must still allow + /api/chat/start to look up the session by its returned session_id.""" + s = new_session() + found = get_session(s.session_id) + assert found is s, ( + "get_session must return the in-memory unsaved session — _handle_chat_start " + "depends on this for the very first message in a fresh session." + ) + + +# ── 2. unsaved sessions never surface in the sidebar ───────────────────────── + + +def test_unsaved_empty_session_hidden_from_sidebar(_isolate): + """all_sessions filters Untitled+0-message regardless of save state (#1171).""" + s = new_session() + ids = {row["session_id"] for row in all_sessions()} + assert s.session_id not in ids, ( + "An unsaved empty Untitled session must not appear in /api/sessions" + ) + + +# ── 3. save() materialises the file when state is real ───────────────────── + + +def test_save_writes_to_disk_when_first_invoked(_isolate): + """The first save() (typically from _handle_chat_start after appending a + user message) creates the JSON file.""" + s = new_session() + assert not s.path.exists() + s.messages.append({"role": "user", "content": "hello"}) + s.save() + assert s.path.exists(), "save() must create the file once it's called" + content = json.loads(s.path.read_text(encoding="utf-8")) + assert content["session_id"] == s.session_id + assert content["messages"] and content["messages"][0]["role"] == "user" + + +def test_btw_background_pattern_still_persists(_isolate): + """btw / background agents at api/routes.py call save() right after + populating title/messages — that path must continue to write to disk + even though new_session itself no longer saves.""" + s = new_session() + s.title = "btw: question" + s.messages = [{"role": "user", "content": "hi"}] + s.save() # mirrors api/routes.py:_handle_btw / _handle_background + assert s.path.exists() + on_disk = json.loads(s.path.read_text(encoding="utf-8")) + assert on_disk["title"] == "btw: question" + + +# ── 4. crash-safety semantics: no orphan files accumulate on the new path ── + + +def test_repeated_new_session_creates_no_disk_files(_isolate, tmp_path): + """Five news in a row produce zero disk files. Pre-fix this would have + written five orphan JSON files to SESSION_DIR.""" + session_dir = tmp_path / "sessions" + for _ in range(5): + new_session() + on_disk_jsons = [p for p in session_dir.glob("*.json") if not p.name.startswith("_")] + assert on_disk_jsons == [], ( + f"new_session() produced disk files: {[p.name for p in on_disk_jsons]}" + ) diff --git a/tests/test_provider_mismatch.py b/tests/test_provider_mismatch.py index cb8bee6ed87..85c666ca5eb 100644 --- a/tests/test_provider_mismatch.py +++ b/tests/test_provider_mismatch.py @@ -541,6 +541,16 @@ def test_api_session_is_side_effect_free_for_stale_models(): sid = created["session"]["session_id"] session_path = TEST_STATE_DIR / "sessions" / f"{sid}.json" + # POST /api/session/new no longer eagerly writes empty sessions to disk + # (#1171 follow-up). Materialise the file from the API response so the + # rest of this test, which checks that GET is side-effect-free against + # an on-disk session with a stale model, has a file to work with. + if not session_path.exists(): + session_path.parent.mkdir(parents=True, exist_ok=True) + session_path.write_text( + json.dumps(created["session"], ensure_ascii=False, indent=2), + encoding="utf-8", + ) session_data = json.loads(session_path.read_text(encoding="utf-8")) stale_model = "google/gemini-3.1-pro-preview" session_data["model"] = stale_model