From 2b537ffa1b418e6859e156a7d7f849e5a5719fd7 Mon Sep 17 00:00:00 2001 From: Frank Song Date: Thu, 14 May 2026 19:31:25 +0800 Subject: [PATCH] Fix archive metadata-only session reload --- CHANGELOG.md | 2 ++ api/routes.py | 10 ++++++ tests/test_metadata_save_wipe_1558.py | 48 ++++++++++++++++++++++++++- 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6df1a29..c861b84b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ### Fixed +- Archive Session (refs #2243) now reloads a cached metadata-only session before mutating `archived`, so the #1558 metadata-only save guard still prevents message wipes without making sidebar archive actions fail with `Archive failed`. + - **PR #2234** by @Jordan-SkyLF (refines #2207) — Three update-banner improvements: (1) Update summaries no longer repeat the same bullet under both "What you'll notice" and "Worth knowing" — visible notice items keep priority, and the secondary section is omitted when there is no distinct detail to show. (2) Update summaries now cap large commit lists (24 + probe item) before sending them to the summarizer and disclose when the summary uses only the latest commit subjects, while keeping the full comparison link available — bounds summarizer cost on large update ranges while remaining honest about coverage. (3) Update banners now wrap generated-summary links and long update text on narrow/mobile screens inside the banner instead of pushing controls off-screen. 108-line regression coverage for short-target dedup, repeated Agent-summary bullets, large-range capped input, and responsive wrapping. - **PR #2236** by @jasonjcwu — Silent failure detection in `api/streaming.py` now scans only NEW messages, not the full conversation history. Pre-fix, the `_assistant_added` check at `_run_agent_streaming` scanned all messages in `result["messages"]` (including pre-turn history); if any prior turn contained an assistant response, `_assistant_added` was `True` and the apperror SSE event was silently skipped, leaving the user staring at a blank response after a provider 401/429/rate-limit error. Fix extracts a `_has_new_assistant_reply(all_messages, prev_count)` helper that only inspects messages beyond the pre-turn history offset (`_previous_context_messages`); applied to both the main detection path and the self-heal/retry `_heal_ok` check. 15-test regression suite covering empty/short/long-history scenarios, the heal path, and the `len < prev_count` edge-case fallback. Also includes a small alignment fix to `test_issue1857_usage_overwrite.py` so the FakeAgent message shape matches what the real agent produces. diff --git a/api/routes.py b/api/routes.py index dadc17f4..43742684 100644 --- a/api/routes.py +++ b/api/routes.py @@ -5111,6 +5111,16 @@ def handle_post(handler, parsed) -> bool: sid = body["session_id"] try: s = get_session(sid) + # #1558: save() refuses metadata-only session stubs because their + # messages list is intentionally empty. If a sidebar/status preload + # left one in the LRU cache, upgrade to a full disk load before + # mutating archived state so the guard stays intact. + if getattr(s, "_loaded_metadata_only", False): + s = Session.load(sid) + if s is None: + raise KeyError(sid) + with LOCK: + SESSIONS[sid] = s except KeyError: cli_meta = _lookup_cli_session_metadata(sid) if not cli_meta: diff --git a/tests/test_metadata_save_wipe_1558.py b/tests/test_metadata_save_wipe_1558.py index ce1b76cc..99246084 100644 --- a/tests/test_metadata_save_wipe_1558.py +++ b/tests/test_metadata_save_wipe_1558.py @@ -130,6 +130,53 @@ def test_clear_stale_stream_state_preserves_messages(temp_session_dir): ) +def test_archive_route_reloads_metadata_only_cached_session(temp_session_dir, monkeypatch): + """Archiving must upgrade cached metadata-only stubs before save().""" + from types import SimpleNamespace + + import api.routes as routes + from api.models import LOCK, SESSIONS, Session, get_session + monkeypatch.setattr(routes, "SESSIONS", SESSIONS) + + sid = _make_session_on_disk(temp_session_dir, n_msgs=12, with_active_stream=False) + stub = get_session(sid, metadata_only=True) + assert getattr(stub, "_loaded_metadata_only", False) is True + assert stub.messages == [] + + # Reproduce the bad cache state: get_session() returns cached entries before + # considering the requested load mode, so a metadata-only stub in SESSIONS + # used to flow straight into archive mutation and hit the #1558 save guard. + with LOCK: + SESSIONS[sid] = stub + + captured = {} + monkeypatch.setattr(routes, "_check_csrf", lambda handler: True) + monkeypatch.setattr(routes, "read_body", lambda handler: {"session_id": sid, "archived": True}) + monkeypatch.setattr( + routes, + "j", + lambda handler, payload, status=200, extra_headers=None: captured.update( + payload=payload, + status=status, + ) + or True, + ) + + assert routes.handle_post(object(), SimpleNamespace(path="/api/session/archive")) is True + + assert captured["status"] == 200 + assert captured["payload"]["session"]["archived"] is True + + reloaded = Session.load(sid) + assert reloaded.archived is True + assert len(reloaded.messages) == 12 + + with LOCK: + cached = SESSIONS[sid] + assert getattr(cached, "_loaded_metadata_only", False) is False + assert len(cached.messages) == 12 + + def test_save_writes_bak_when_messages_shrink(temp_session_dir): """The backup safeguard: a save that shrinks messages must leave a .bak.""" from api.models import Session @@ -323,4 +370,3 @@ def test_msg_count_returns_neg1_for_non_dict_top_level(temp_session_dir): list_shaped.write_text(json.dumps([{"session_id": "x"}]), encoding="utf-8") # Pre-fix: AttributeError. Post-fix: -1. assert _msg_count(list_shaped) == -1 -