harden #5504 on rodboev's latest head: complete the tombstone wiring + CHANGELOG
@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 <rodboev@users.noreply.github.com>
This commit is contained in:
1 parent
06217c93b8
commit
63d0c00f8d
7 files changed
+176
-9
No files matched your search
@@ -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)
|
||||
|
||||
@@ -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':
|
||||
|
||||
+11
-1
@@ -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 {}
|
||||
|
||||
+35
-3
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <sid>.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
|
||||
|
||||
@@ -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_audit_reports_deleted_webui_tombstone_is_unsafe(tmp_path):
|
||||
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"] == 1
|
||||
assert (tmp_path / f"{sid}.json").exists()
|
||||
|
||||
|
||||
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,7 +103,8 @@ 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")
|
||||
|
||||
assert any(
|
||||
@@ -93,6 +122,8 @@ def test_audit_reports_deleted_webui_tombstone_is_unsafe(tmp_path):
|
||||
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):
|
||||
|
||||
Reference in new issue
Block a user