From 161e4173c29fc8146b9760cb2c4636f11d7d1049 Mon Sep 17 00:00:00 2001 From: nanw Date: Wed, 1 Jul 2026 17:26:56 +0800 Subject: [PATCH] fix(sessions): prune index-only ghost sessions (#5331) Phase 2 of cleanup now sweeps _index.json for stale entries with no backing file and no in-memory session, removing them regardless of title (catches multi-language 'Untitled' variants). Phase 3 only deletes the index when Phase 1 removed files AND Phase 2 couldn't run, fixing the cache-busting on every cleanup call. Fix two phase-interaction bugs flagged by Greptile review: - Track phase1_removed_ids to prevent Phase 2 double-counting sessions already removed from disk by Phase 1. - Track phase2_rewrote_index so Phase 3 skips deletion when Phase 2 already cleaned the index in-place. Adds test_issue5331_index_only_ghost_cleanup.py with 12 test cases. --- api/routes.py | 80 ++++ ...test_issue5331_index_only_ghost_cleanup.py | 419 ++++++++++++++++++ 2 files changed, 499 insertions(+) create mode 100644 tests/test_issue5331_index_only_ghost_cleanup.py diff --git a/api/routes.py b/api/routes.py index 74a978fb..120419e7 100644 --- a/api/routes.py +++ b/api/routes.py @@ -18015,6 +18015,9 @@ def _handle_memory_read(handler, parsed=None): def _handle_sessions_cleanup(handler, body, zero_only=False): cleaned = 0 + phase1_removed_ids = set() + + # Phase 1: Clean orphan session files (existing behavior). for p in SESSION_DIR.glob("*.json"): if p.name.startswith("_"): continue @@ -18029,10 +18032,87 @@ def _handle_sessions_cleanup(handler, body, zero_only=False): SESSIONS.pop(p.stem, None) p.unlink(missing_ok=True) cleaned += 1 + phase1_removed_ids.add(p.stem) except Exception: logger.debug("Failed to clean up session file %s", p) + + phase1_touched = bool(cleaned) + phase2_rewrote_index = False + + # Phase 2: Index-only ghost sweep (#5331). + # Remove index entries that have no backing .json file and no + # in-memory session. These are orphaned rows left by a write path that + # updated _index.json without writing the session sidecar. + # + # Title-agnostic (#5331): a session with no backing file has no real + # data regardless of its title, so even a non-"Untitled" stub in the + # index is a ghost. A legitimate session always has a sidecar file. + # + # Holds _INDEX_WRITE_LOCK for the full read-modify-write cycle to + # prevent races with concurrent Session.save() / prune_session_from_index(). if SESSION_INDEX_FILE.exists(): + try: + from api.models import _INDEX_WRITE_LOCK, _safe_replace + + with _INDEX_WRITE_LOCK: + index_file_data = json.loads( + SESSION_INDEX_FILE.read_text(encoding="utf-8") + ) + if isinstance(index_file_data, list): + live_ids = { + p.stem + for p in SESSION_DIR.glob("*.json") + if not p.name.startswith("_") + } + with LOCK: + in_memory_ids = set(SESSIONS.keys()) + + survivors = [] + for entry in index_file_data: + sid = entry.get("session_id") + if not sid or sid in live_ids or sid in in_memory_ids: + survivors.append(entry) + continue + # Phase 1 already removed the backing file for this + # sid, so the index entry is stale too. Drop it + # from the index without double-counting. + if sid in phase1_removed_ids: + continue + # Index-only ghost — no backing file, not in memory. + cleaned += 1 + # Ghost not added to survivors — removed from index. + + if cleaned > 0 and len(survivors) < len(index_file_data): + _tmp = SESSION_INDEX_FILE.with_suffix( + f".tmp.{os.getpid()}.{threading.current_thread().ident}" + ) + _payload = json.dumps(survivors, ensure_ascii=False, indent=2) + try: + with open(_tmp, "w", encoding="utf-8") as f: + f.write(_payload) + f.flush() + os.fsync(f.fileno()) + _safe_replace(_tmp, SESSION_INDEX_FILE) + phase2_rewrote_index = True + except Exception: + try: + _tmp.unlink(missing_ok=True) + except Exception: + pass + raise + except Exception: + logger.debug( + "Failed to clean up index-only session entries", exc_info=True + ) + + # Post-cleanup index invalidation. + # When Phase 1 removed files and Phase 2 didn't already clean the + # index, delete the index to force a fresh rebuild from disk on the + # next sidebar poll. When Phase 2 succeeded the index is already + # correct, so keep it (avoids a wasteful rebuild). + if phase1_touched and not phase2_rewrote_index and SESSION_INDEX_FILE.exists(): SESSION_INDEX_FILE.unlink(missing_ok=True) + return j(handler, {"ok": True, "cleaned": cleaned}) diff --git a/tests/test_issue5331_index_only_ghost_cleanup.py b/tests/test_issue5331_index_only_ghost_cleanup.py new file mode 100644 index 00000000..efcfb32e --- /dev/null +++ b/tests/test_issue5331_index_only_ghost_cleanup.py @@ -0,0 +1,419 @@ +"""Regression coverage for index-only ghost session cleanup (#5331). + +#5331: sessions created in ``_index.json`` without a backing ``.json`` sidecar +file accumulate in the sidebar and cannot be removed via normal UI. The existing +``_handle_sessions_cleanup`` only iterates ``SESSION_DIR.glob('*.json')`` — +structurally blind to index-only entries. + +Phase 2 (new) reads the index and prunes entries that have no backing file +and no in-memory session. A legitimate session always has either a file on +disk or an in-memory ``SESSIONS`` entry (or both). +""" + +from __future__ import annotations + +import io +import json +import sys +import threading +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parent.parent +if str(REPO) not in sys.path: + sys.path.insert(0, str(REPO)) + + +@pytest.fixture +def mock_env(tmp_path, monkeypatch): + """Set up an isolated SESSION_DIR with monkeypatched globals. + + Patches at the ``api.config`` level so that ``api.routes`` AND + ``api.models`` (used by ``Session.load`` in Phase 1) both read + the test temp paths. + """ + import api.config as config_mod + import api.routes as routes + import api.models as models + + sessions_dir = tmp_path / "sessions" + sessions_dir.mkdir() + index_file = sessions_dir / "_index.json" + + # Patch at the config source so all importers see the same test paths. + monkeypatch.setattr(config_mod, "SESSION_DIR", sessions_dir) + monkeypatch.setattr(config_mod, "SESSION_INDEX_FILE", index_file) + monkeypatch.setattr(config_mod, "SESSIONS", {}) + monkeypatch.setattr(routes, "LOCK", threading.Lock()) + # Also refresh the module-level aliases in routes and models so they + # pick up the patched config values. + monkeypatch.setattr(routes, "SESSION_DIR", sessions_dir) + monkeypatch.setattr(routes, "SESSION_INDEX_FILE", index_file) + monkeypatch.setattr(routes, "SESSIONS", {}) + monkeypatch.setattr(models, "SESSION_DIR", sessions_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", index_file) + return sessions_dir, index_file + + +def _fake_handler(): + """Minimal handler mock for ``_handle_sessions_cleanup``. + + ``j()`` writes the JSON response to ``handler.wfile`` (a ``BytesIO``), + so callers read the result via ``handler.wfile.getvalue()``. + """ + handler = type("FakeHandler", (), {})() + handler.wfile = io.BytesIO() + handler.send_response = lambda status: None + handler.send_header = lambda key, value: None + handler.end_headers = lambda: None + return handler + + +def _fake_handler_result(handler): + """Parse the JSON written to ``handler.wfile`` by ``j()``.""" + return json.loads(handler.wfile.getvalue()) + + +def _make_session_file(sessions_dir, sid, title="Untitled", **extra): + """Write a ``.json`` session file and return the compact entry dict. + + Uses the minimal Session-serializable format: ``session_id`` is required, + all other fields have defaults. ``**extra`` is folded directly into + the JSON so callers can set ``message_count``, ``created_at``, etc. + """ + entry = {"session_id": sid, "title": title, **extra} + (sessions_dir / f"{sid}.json").write_text(json.dumps(entry)) + return entry + + +def _write_index(index_file, entries): + """Write the session index file.""" + index_file.write_text(json.dumps(entries, indent=2)) + + +def _read_index(index_file): + """Read the session index file, returning empty list on missing/corrupt.""" + if not index_file.exists(): + return [] + try: + return json.loads(index_file.read_text(encoding="utf-8")) + except Exception: + return [] + + +# ── Phase 2: index-only ghost sweep (#5331) ──────────────────────────────── + + +def test_cleanup_prunes_index_only_ghosts(mock_env): + """Index-only Untitled entries with no backing file are pruned.""" + sessions_dir, index_file = mock_env + + # Legitimate sessions with backing files — should survive. + _make_session_file(sessions_dir, "sess-a", "Legit") + _make_session_file(sessions_dir, "sess-b", "Production") + + # Ghost entry: exists only in index, no backing file. + _write_index(index_file, [ + {"session_id": "sess-a", "title": "Legit", "message_count": 5}, + {"session_id": "sess-b", "title": "Production", "message_count": 42}, + {"session_id": "sess-ghost-1", "title": "Untitled", "message_count": 3}, + ]) + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + + assert result["ok"] is True + assert result["cleaned"] == 1 # only the ghost + + updated = _read_index(index_file) + sids = {e["session_id"] for e in updated} + assert "sess-ghost-1" not in sids + assert "sess-a" in sids + assert "sess-b" in sids + + +def test_cleanup_keeps_in_memory_ghosts(mock_env): + """Index-only entries still in memory (SESSIONS) are kept.""" + sessions_dir, index_file = mock_env + import api.routes as routes + + # One entry in-memory but no backing file — should survive Phase 2. + routes.SESSIONS["sess-mem"] = "placeholder" + _write_index(index_file, [ + {"session_id": "sess-mem", "title": "Untitled", "message_count": 2}, + ]) + + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 0 + + updated = _read_index(index_file) + assert any(e["session_id"] == "sess-mem" for e in updated) + + +def test_cleanup_mixed_ghosts_and_legit(mock_env): + """Mix of ghost, file-backed, and in-memory entries — only ghosts pruned.""" + sessions_dir, index_file = mock_env + import api.routes as routes + + # File-backed (survive Phase 1 — not Untitled, or has messages) + _make_session_file(sessions_dir, "sess-1", "Chat") + _make_session_file(sessions_dir, "sess-2", "Chat2") + + # In-memory (survive) + routes.SESSIONS["sess-3"] = "placeholder" + + # Ghosts (pruned) + ghost_1 = {"session_id": "sess-ghost-a", "title": "Untitled", "message_count": 5} + ghost_2 = {"session_id": "sess-ghost-b", "title": "Something", "message_count": 0} + + _write_index(index_file, [ + {"session_id": "sess-1", "title": "Chat", "message_count": 10}, + {"session_id": "sess-2", "title": "Untitled", "message_count": 1}, + {"session_id": "sess-3", "title": "Untitled", "message_count": 2}, + ghost_1, + ghost_2, + ]) + + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 2 + + updated = _read_index(index_file) + sids = {e["session_id"] for e in updated} + assert "sess-ghost-a" not in sids + assert "sess-ghost-b" not in sids + assert "sess-1" in sids + assert "sess-2" in sids + assert "sess-3" in sids + + +def test_cleanup_empty_index_does_not_crash(mock_env): + """An index with no entries leaves Phase 2 a no-op.""" + sessions_dir, index_file = mock_env + _write_index(index_file, []) + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 0 + + +def test_cleanup_no_index_file_does_not_crash(mock_env): + """No index file at all — Phase 2 is skipped, no error.""" + sessions_dir, index_file = mock_env + assert not index_file.exists() + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 0 + + +def test_cleanup_corrupt_index_does_not_crash(mock_env): + """A corrupt index file is caught by the try/except in Phase 2.""" + sessions_dir, index_file = mock_env + index_file.write_text("not valid json") + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 0 # Phase 2 gracefully skipped + + # Index file still exists and still invalid. + assert index_file.exists() + assert index_file.read_text() == "not valid json" + + +def test_cleanup_ghost_without_session_id_field(mock_env): + """An index entry missing ``session_id`` is skipped (not treated as ghost).""" + sessions_dir, index_file = mock_env + _make_session_file(sessions_dir, "sess-real", "Real") + + _write_index(index_file, [ + {"session_id": "sess-real", "title": "Real", "message_count": 1}, + {"title": "NoId", "message_count": 3}, # missing session_id + ]) + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 0 + + updated = _read_index(index_file) + assert len(updated) == 2 # both kept + + +def test_cleanup_index_only_empty_ghosts_without_explicit_messages(mock_env): + """Index-only ghost with no ``messages`` field is still pruned. + + The minimal index entry ``{session_id, title}`` works — Phase 2 does not + try to load the session (there's no file), so it just checks sid presence + in ``live_ids`` and ``in_memory_ids``. + """ + sessions_dir, index_file = mock_env + _make_session_file(sessions_dir, "sess-real", "Real") + + _write_index(index_file, [ + {"session_id": "sess-real", "title": "Real", "message_count": 1}, + {"session_id": "sess-ghost-minimal"}, + ]) + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + assert result["cleaned"] == 1 + + updated = _read_index(index_file) + assert len(updated) == 1 + assert updated[0]["session_id"] == "sess-real" + + +# ── Phase 3: index cache thrash fix ────────────────────────────────────────── + + +def test_cleanup_deletes_index_only_when_phase1_touched(mock_env): + """Phase 3 only unlinks _index.json when Phase 1 removed files. + + Phase 2 always writes the index in-place. The post-cleanup + ``unlink(missing_ok=True)`` should only fire when Phase 1 removed + actual session files, because that's the only case where the index + was NOT already written in-place by Phase 2. + """ + sessions_dir, index_file = mock_env + import api.routes as routes + + # Only ghosts in index — Phase 1 touches nothing, Phase 2 writes in-place. + _write_index(index_file, [ + {"session_id": "sess-ghost", "title": "Untitled", "message_count": 1}, + ]) + + routes._handle_sessions_cleanup(_fake_handler(), {}) + + # After Phase-2-write, index should exist (Phase 3 should not have unlinked it). + assert index_file.exists() + + # Verify it was cleaned too + updated = _read_index(index_file) + assert len(updated) == 0 + + +def test_cleanup_index_rewritten_when_phase1_removed_files(mock_env): + """Phase 2 rewrites the index when Phase 1 removes files. + + Phase 2 handles the stale index entries left by Phase 1 removals, + so Phase 3 does NOT delete the index — it's already clean. + """ + sessions_dir, index_file = mock_env + import api.routes as routes + + # A file-backed Untitled-0-message session — Phase 1 target. + _make_session_file(sessions_dir, "sess-orphan", "Untitled") + _write_index(index_file, [ + {"session_id": "sess-orphan", "title": "Untitled", "message_count": 0}, + ]) + + routes._handle_sessions_cleanup(_fake_handler(), {}, zero_only=False) + + # Phase 1 unlinked the file. Phase 2 removed the stale index entry. + # Phase 3 skipped because Phase 2 already cleaned the index. + # Index exists with zero entries (clean). + assert index_file.exists() + assert _read_index(index_file) == [] + + +def test_cleanup_phase3_deletes_when_phase2_could_not_run(mock_env): + """Phase 3 still deletes the index when Phase 2 couldn't run. + + When the index is corrupt (or missing), Phase 2 gracefully skips. + Phase 1 has removed files, leaving no way to clean the stale index, + so Phase 3 deletes it for a fresh rebuild. + """ + sessions_dir, index_file = mock_env + import api.routes as routes + + # Phase 1 target: file-backed session with zero messages. + _make_session_file(sessions_dir, "sess-orphan", "Untitled") + # Corrupt index — Phase 2 will catch the exception and skip. + index_file.write_text("corrupt json") + + routes._handle_sessions_cleanup(_fake_handler(), {}, zero_only=False) + + # Phase 1 removed the file. Phase 2 caught the corrupt-json exception. + # Phase 3: phase1_touched=True, phase2_rewrote_index=False → unlink. + assert not index_file.exists() + + +def test_cleanup_no_double_count_when_phase1_and_phase2_overlap(mock_env): + """Phase 2 does not double-count sessions already removed by Phase 1. + + When Phase 1 removes a file-backed session, the corresponding index + entry becomes stale. Phase 2 sees it (no file, not in memory) but + recognises it as a Phase-1 removal via ``phase1_removed_ids`` and + skips counting it — preventing ``cleaned`` from inflating. + """ + sessions_dir, index_file = mock_env + import api.routes as routes + + # Phase 1 target: file-backed session with zero messages. + _make_session_file(sessions_dir, "sess-zero", "Untitled") + # Phase 2 target: index-only ghost. + _write_index(index_file, [ + {"session_id": "sess-zero", "title": "Untitled", "message_count": 0}, + {"session_id": "sess-ghost", "title": "Untitled", "message_count": 1}, + ]) + + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}) + result = _fake_handler_result(handler) + + # Phase 1: 1 (sess-zero). Phase 2: 1 (sess-ghost). Total = 2. + assert result["cleaned"] == 2 + + # Index exists (Phase 2 rewrote it, Phase 3 skipped). + assert index_file.exists() + assert _read_index(index_file) == [] + + +# ── zero_only mode preserves Phase 2 ───────────────────────────────────────── + + +def test_cleanup_zero_only_still_runs_phase2(mock_env): + """Phase 2 (ghost sweep) runs regardless of ``zero_only`` flag. + + Phase 2 handles the stale index entry from Phase 1's removal, so the + index survives in a clean state (empty) and no double-counting occurs. + """ + sessions_dir, index_file = mock_env + + # Phase-1 target: a real file with zero messages + _make_session_file(sessions_dir, "sess-zero", "SomeTitle") + + # Ghost in index only + _write_index(index_file, [ + {"session_id": "sess-zero", "title": "SomeTitle", "message_count": 0}, + {"session_id": "sess-ghost", "title": "Untitled", "message_count": 1}, + ]) + + import api.routes as routes + handler = _fake_handler() + routes._handle_sessions_cleanup(handler, {}, zero_only=True) + result = _fake_handler_result(handler) + + # Phase 1: 1 (sess-zero, zero messages). Phase 2: 1 (sess-ghost). Total = 2. + assert result["ok"] is True + assert result["cleaned"] == 2 + + # Index exists (Phase 2 rewrote it, Phase 3 skipped). + assert index_file.exists() + assert _read_index(index_file) == []