From 63d0c00f8dcb8dc405dd3abad6dfbc775e6ed212 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sat, 4 Jul 2026 05:32:51 +0000 Subject: [PATCH] harden #5504 on rodboev's latest head: complete the tombstone wiring + CHANGELOG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @rodboev independently pushed fixes for messaging-tombstone (matches ours) and an audit orphan-.bak skip — but the audit skip used _marks_deleted_webui_session (index heuristic), which re-introduces the crash-suppression bug, and the other 4 resurrection/recovery paths were still open. Rebuilt the stage on rodboev's new head (fc21c3f3, attribution preserved) and applied the complete superset: - routes.py claim path: tombstone 404 gated on _state_db_session_source in ('','webui','fork') so foreign-source rows always materialize (finding B) - models.py sidebar projection: skip tombstoned source='webui' rows w/ no live sidecar so a deleted session can't resurface in /api/sessions (finding C) - session_recovery.py orphan-.bak startup: skip restore when durable tombstone present, durable-only (finding D) - session_recovery.py audit orphan-.bak: durable-only classify as deleted_session_skipped (corrects rodboev's index-heuristic version) - session_recovery.py _read_state_db_missing_sidecar_rows: durable-tombstone-only so a genuine crash (index intact, no durable tombstone) still repairs (finding F) Regression tests (all verified fail-without-fix): sidebar-projection, orphan-.bak startup skip, messaging-not-tombstoned, index-only-crash-repairable. Both advisors (Codex SAFE TO SHIP + Opus ship-with-fixes) cleared the prior stage; this rebuild carries the same fixes onto the contributor's latest work. Co-authored-by: rodboev --- CHANGELOG.md | 1 + api/models.py | 18 +++++ api/routes.py | 12 ++- api/session_recovery.py | 38 +++++++++- tests/test_gateway_sync.py | 47 ++++++++++++ tests/test_metadata_save_wipe_1558.py | 28 +++++++ .../test_session_db_sidecar_reconciliation.py | 75 +++++++++++++------ 7 files changed, 193 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06feb461..80670d28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ ### Fixed +- **A deleted conversation no longer comes back as a read-only "ghost" after a restart.** Deleting a WebUI session removed its file but left a recoverable row in the agent's state database, so after a server restart and page reload the deleted conversation could reappear as a read-only "Agent" session still showing its old transcript. Deleted WebUI sessions are now recorded in a durable tombstone that the sidebar projection, startup backup-recovery, audit, and session-claim paths all honor, so a deleted session stays gone (the backend keeps returning the 404 the client relies on to clear the stale route). Genuine crash recovery is unaffected — a session whose sidecar is lost without a delete is still restored — imported external-channel (messaging) sessions are exempt and keep their transcript, and a deleted id is automatically un-tombstoned if the same session is later re-created or re-imported. Thanks @rodboev. (#5504, #5498) - **A drag that ends over the sidebar no longer switches conversations by accident.** Releasing the mouse (a stray `pointerup`) over a session row after a drag that began elsewhere in the main content area could switch you into that conversation unintentionally. Session switching now ignores a `pointerup` that isn't a genuine click on the row. Thanks @webtecnica. (#5487, #5462) - **Extension gallery downloads no longer hang on networks with broken IPv6.** Gallery installs and registry fetches followed the OS address order (IPv6-first); on a network advertising dead IPv6 routes, every install stalled for the full TCP timeout before falling back to IPv4. Downloads now try IPv4 first (via a non-global, thread-safe per-connection override that preserves the existing redirect allowlist and timeouts), so installs are fast again on such networks. Thanks @rumotoshino. (#5458) - **Switching profiles no longer leaves you unable to upload in a fresh chat, and cross-profile sidebar rows open correctly.** When you switched profiles with an empty current chat, the browser could keep the previous profile's session id — so a later file upload posted a stale id and failed with "Session not found" until a hard reload. The empty chat is now replaced during the switch when it belonged to a different profile, so uploads work immediately. Additionally, clicking a session that belongs to another profile now switches to its owning profile before loading it (instead of 404ing). Thanks @ruizanthony. (#5460) diff --git a/api/models.py b/api/models.py index ceae3697..317f9f27 100644 --- a/api/models.py +++ b/api/models.py @@ -5756,6 +5756,16 @@ def _load_cli_sessions_uncached( return None profile_value = _cli_profile or 'default' + # A deleted WebUI session is tombstoned (see _record_webui_deleted_session_tombstone) + # so recovery/audit/claim treat it as gone. The sidebar's own state.db projection + # must honor the same tombstone, or a deleted WebUI session reappears here as an + # "Agent" ghost the moment non-WebUI sessions are shown (#5498, second path). Only + # suppress genuine WebUI rows with no live sidecar — a re-created/re-imported sid + # (live {sid}.json) always beats a stale tombstone. + try: + _deleted_webui_tombstone = _load_webui_deleted_session_tombstone() + except Exception: + _deleted_webui_tombstone = frozenset() for row in read_importable_agent_session_rows( db_path, limit=visible_session_limit if visible_session_limit is not None else ( @@ -5774,6 +5784,14 @@ def _load_cli_sessions_uncached( profile = profile_value # CLI DB has no profile column; use active profile _source = row['source'] or 'cli' + # Honor the deleted-WebUI tombstone: a WebUI row the user deleted must + # not resurface in this projection (the #5498 ghost). Live sidecar wins. + if ( + _source == 'webui' + and sid in _deleted_webui_tombstone + and not (SESSION_DIR / f"{sid}.json").exists() + ): + continue _source_meta = normalize_agent_session_source(_source) _title = row['title'] if not _title and _source == 'cron': diff --git a/api/routes.py b/api/routes.py index aefee8e5..35393dc4 100644 --- a/api/routes.py +++ b/api/routes.py @@ -7390,7 +7390,10 @@ def _claim_or_synthesize_cli_session(sid: str, cli_meta: dict = None): if ( ( _session_index_marks_was_webui(sid) - or _session_deleted_tombstone_marks_was_webui(sid) + or ( + _session_deleted_tombstone_marks_was_webui(sid) + and _state_db_session_source(sid) in ("", "webui", "fork") + ) ) and not _is_subagent_child_session_id(sid) ): @@ -7400,6 +7403,13 @@ def _claim_or_synthesize_cli_session(sid: str, cli_meta: dict = None): # Those must recover their transcript from state.db below rather than # 404 as a genuinely-deleted WebUI session (#5307). Every other # index-marked-WebUI id keeps the #2782 self-heal 404 contract. + # + # The durable delete tombstone only 404s a row that is WebUI-owned + # (source webui/fork, or blank = a stale URL with no surviving row). + # A foreign-source row (messaging/cli/tui/desktop) that happens to + # carry a tombstone — e.g. a WebUI delete of an imported session whose + # state.db row the external writer later re-created — must still + # materialize its transcript, never be self-healed to a 404 (#5504). return None, "was_webui" if cli_meta is None: cli_meta = _lookup_cli_session_metadata(sid) or {} diff --git a/api/session_recovery.py b/api/session_recovery.py index 7a17b427..9c1c35d2 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -330,6 +330,18 @@ def _orphaned_backup_live_paths( if _msg_count(bak_path) < 0: continue session_id = live_path.stem + # A WebUI session the user deleted must not be resurrected from its + # surviving .bak on the next boot (#5498). Use the DURABLE tombstone + # only — not the _index.json heuristic — so a genuine crash that loses + # the sidecar while its index entry survives is still restored (the + # crash-recovery case this helper exists for). + if _durable_tombstone_marks_deleted_webui_session(session_dir, session_id): + logger.info( + "recover_all_sessions_on_startup: skipped orphan backup %s; " + "session is tombstoned as a deleted WebUI session", + bak_path.name, + ) + continue if not _state_db_has_session(session_id, state_db_path): logger.info( "recover_all_sessions_on_startup: skipped orphan backup %s; " @@ -383,7 +395,15 @@ def _read_state_db_missing_sidecar_rows( sid = str(data.get('id') or '').strip() if not sid or (session_dir / f"{sid}.json").exists(): continue - tombstoned = _marks_deleted_webui_session(session_dir, sid) + # Only the DURABLE delete tombstone suppresses state.db sidecar + # repair. The _index.json heuristic must NOT gate this path: a + # genuine crash that loses the sidecar while its index entry + # survives (no durable tombstone) is exactly the case this repair + # exists for, and on master it materialized the sidecar. Using + # _marks_deleted_webui_session() here (which ORs in the index + # heuristic) wrongly classified that crash as a delete and + # stopped recovery (#5504 Codex/Opus finding). + tombstoned = _durable_tombstone_marks_deleted_webui_session(session_dir, sid) if tombstoned and not include_empty: continue message_rows: list[dict] = [] @@ -694,8 +714,20 @@ def audit_session_recovery(session_dir: Path, state_db_path: Path | None = None) items.append(_new_audit_item( session_id, "malformed_orphan_backup", "unsafe_to_repair", "manual_review", -1, bak_messages )) - elif _marks_deleted_webui_session(session_dir, session_id): - continue + elif _durable_tombstone_marks_deleted_webui_session(session_dir, session_id): + # The user deleted this WebUI session; its surviving .bak must NOT be + # reported as repairable. DURABLE tombstone only — the _index.json + # heuristic must not suppress a genuine crash whose index survived + # (that case is legitimately repairable). Matches the startup-recovery + # skip + the state.db recovery path (#5504 Codex/Opus finding). + items.append(_new_audit_item( + session_id, + "state_db_deleted_webui_tombstone", + "unsafe_to_repair", + "deleted_session_skipped", + -1, + bak_messages, + )) elif _state_db_has_session(session_id, state_db_path): items.append(_new_audit_item( session_id, "orphan_backup", "repairable", "restore_from_bak", -1, bak_messages diff --git a/tests/test_gateway_sync.py b/tests/test_gateway_sync.py index ff8eb600..6aeb3425 100644 --- a/tests/test_gateway_sync.py +++ b/tests/test_gateway_sync.py @@ -1922,6 +1922,16 @@ def test_delete_imported_messaging_session_preserves_agent_memory(cleanup_test_s (sid,), ).fetchone()[0] assert remaining == 2 + + # A messaging-session delete deliberately preserves the state.db + # transcript (delete_cli_session is skipped), so it must NOT be + # recorded in the deleted-WebUI tombstone — otherwise + # _claim_or_synthesize_cli_session() would treat the still-live + # channel session as was-webui and self-heal it to a 404 on reopen. + import api.models as _m + assert sid not in _m._load_webui_deleted_session_tombstone(), ( + "messaging session must not be tombstoned as a deleted WebUI session" + ) finally: try: _remove_test_sessions(conn, sid) @@ -1930,6 +1940,43 @@ def test_delete_imported_messaging_session_preserves_agent_memory(cleanup_test_s pass +def test_deleted_webui_session_stays_out_of_sidebar_projection(cleanup_test_sessions): + """A tombstoned deleted WebUI session must not resurface via the state.db projection. + + Regression for #5498 (second path): even when non-WebUI sessions are shown, + _load_cli_sessions_uncached() projects source='webui' state.db rows into the + sidebar. Without honoring the deleted-WebUI tombstone, a deleted session + reappears as an 'Agent' ghost. The projection must skip a tombstoned + source='webui' row that has no live sidecar. + """ + import api.models as _m + + conn = _ensure_state_db() + sid = 'webui_deleted_ghost_projection_001' + cleanup_test_sessions.append(sid) + try: + _insert_gateway_session(conn, session_id=sid, source='webui', title='Deleted WebUI Ghost') + _m._record_webui_deleted_session_tombstone(sid) + # Tombstone should win: ensure no live sidecar exists for this sid. + assert not (_m.SESSION_DIR / f'{sid}.json').exists() + + post('/api/settings', {'show_cli_sessions': True}) + data, status = get('/api/sessions') + assert status == 200 + ids = {s['session_id'] for s in data.get('sessions', [])} + assert sid not in ids, ( + "deleted (tombstoned) WebUI session must not resurface in the sidebar projection" + ) + finally: + try: + _m._clear_webui_deleted_session_tombstone(sid) + _remove_test_sessions(conn, sid) + conn.close() + except Exception: + pass + post('/api/settings', {'show_cli_sessions': False}) + + def test_imported_cron_sessions_hidden_from_sidebar_by_default(cleanup_test_sessions): """Cron sessions already imported into the WebUI store should stay hidden from the sidebar.""" from api.models import Session diff --git a/tests/test_metadata_save_wipe_1558.py b/tests/test_metadata_save_wipe_1558.py index 5a694169..4228b668 100644 --- a/tests/test_metadata_save_wipe_1558.py +++ b/tests/test_metadata_save_wipe_1558.py @@ -270,6 +270,34 @@ def test_recover_all_sessions_on_startup_restores_orphan_bak(temp_session_dir): assert len(restored["messages"]) == 293 +def test_recover_all_sessions_on_startup_skips_tombstoned_orphan_bak(temp_session_dir): + """A deleted WebUI session must NOT be resurrected from its surviving .bak. + + Regression for #5498 / #5504: the double-failure delete (backup unlink + + state.db delete both fail) leaves a surviving .json.bak plus the + durable deleted-WebUI tombstone. Startup recovery must honor the tombstone + and NOT recreate the sidecar (otherwise the deleted transcript reappears in + the sidebar on the next boot — the literal ghost the fix exists to kill). + """ + import api.models as _m + + sid = _make_session_on_disk(temp_session_dir, n_msgs=42) + live_path = temp_session_dir / f"{sid}.json" + bak_path = temp_session_dir / f"{sid}.json.bak" + bak_path.write_text(live_path.read_text(encoding="utf-8"), encoding="utf-8") + live_path.unlink() + # Simulate the delete route's durable tombstone for this deleted sid. + _m._record_webui_deleted_session_tombstone(sid) + try: + from api.session_recovery import recover_all_sessions_on_startup + result = recover_all_sessions_on_startup(temp_session_dir) + + assert result["restored"] == 0, "tombstoned orphan .bak must not be restored" + assert not live_path.exists(), "deleted session sidecar must not be recreated on boot" + finally: + _m._clear_webui_deleted_session_tombstone(sid) + + def test_recover_all_sessions_on_startup_rebuilds_missing_index_without_restores(temp_session_dir, monkeypatch): """Startup recovery must rebuild a missing index even when no .bak restore runs.""" import api.models as _m diff --git a/tests/test_session_db_sidecar_reconciliation.py b/tests/test_session_db_sidecar_reconciliation.py index a3be1690..aee60eb4 100644 --- a/tests/test_session_db_sidecar_reconciliation.py +++ b/tests/test_session_db_sidecar_reconciliation.py @@ -48,7 +48,9 @@ def test_recover_missing_sidecars_from_state_db_materializes_webui_row(tmp_path) assert [m["content"] for m in data["messages"]] == ["message 1", "message 2"] -def test_recover_missing_sidecars_from_state_db_skips_deleted_webui_tombstone(tmp_path): +def test_recover_missing_sidecars_from_state_db_skips_deleted_webui_tombstone(tmp_path, monkeypatch): + import api.models as _m + monkeypatch.setattr(_m, "SESSION_DIR", tmp_path) sid = _make_state_db(tmp_path / "state.db", sid="deleted_webui_001") _write_index(tmp_path, [ { @@ -58,14 +60,40 @@ def test_recover_missing_sidecars_from_state_db_skips_deleted_webui_tombstone(tm "session_source": "webui", } ]) + # A genuine delete records the DURABLE tombstone; only that suppresses repair. + _m._record_webui_deleted_session_tombstone(sid) + try: + result = recover_missing_sidecars_from_state_db(tmp_path, tmp_path / "state.db") + assert result["materialized"] == 0 + assert not (tmp_path / f"{sid}.json").exists() + finally: + _m._clear_webui_deleted_session_tombstone(sid) + +def test_recover_missing_sidecars_index_only_no_tombstone_is_repairable(tmp_path, monkeypatch): + """A crash that loses the sidecar (index intact, NO durable tombstone) must + still be recovered from state.db — the index heuristic alone must not + suppress repair (#5504 Codex/Opus finding; matches origin/master behavior).""" + import api.models as _m + monkeypatch.setattr(_m, "SESSION_DIR", tmp_path) + sid = _make_state_db(tmp_path / "state.db", sid="crashed_webui_001") + _write_index(tmp_path, [ + { + "session_id": sid, + "source_tag": "webui", + "raw_source": "webui", + "session_source": "webui", + } + ]) + # No durable tombstone recorded (this is a crash, not a delete). result = recover_missing_sidecars_from_state_db(tmp_path, tmp_path / "state.db") - - assert result["materialized"] == 0 - assert not (tmp_path / f"{sid}.json").exists() + assert result["materialized"] == 1 + assert (tmp_path / f"{sid}.json").exists() -def test_audit_reports_deleted_webui_tombstone_is_unsafe(tmp_path): +def test_audit_reports_deleted_webui_tombstone_is_unsafe(tmp_path, monkeypatch): + import api.models as _m + monkeypatch.setattr(_m, "SESSION_DIR", tmp_path) sid = _make_state_db(tmp_path / "state.db", sid="deleted_webui_001") _write_index(tmp_path, [ { @@ -75,24 +103,27 @@ def test_audit_reports_deleted_webui_tombstone_is_unsafe(tmp_path): "session_source": "webui", } ]) + _m._record_webui_deleted_session_tombstone(sid) + try: + report = audit_session_recovery(tmp_path, state_db_path=tmp_path / "state.db") - report = audit_session_recovery(tmp_path, state_db_path=tmp_path / "state.db") - - assert any( - item["session_id"] == sid - and item["kind"] == "state_db_deleted_webui_tombstone" - and item["category"] == "unsafe_to_repair" - and item["recommendation"] == "deleted_session_skipped" - for item in report["items"] - ) - assert not any( - item["session_id"] == sid and item["kind"] == "state_db_missing_sidecar" - for item in report["items"] - ) - assert not any( - item["session_id"] == sid and item["category"] == "repairable" - for item in report["items"] - ) + assert any( + item["session_id"] == sid + and item["kind"] == "state_db_deleted_webui_tombstone" + and item["category"] == "unsafe_to_repair" + and item["recommendation"] == "deleted_session_skipped" + for item in report["items"] + ) + assert not any( + item["session_id"] == sid and item["kind"] == "state_db_missing_sidecar" + for item in report["items"] + ) + assert not any( + item["session_id"] == sid and item["category"] == "repairable" + for item in report["items"] + ) + finally: + _m._clear_webui_deleted_session_tombstone(sid) def test_recover_missing_sidecars_skips_durable_delete_tombstone_without_index(tmp_path, monkeypatch):