From 52201ac9f7552bd06d67571ec0821f9e7615c0d4 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 14 Aug 2026 09:43:07 -0300 Subject: [PATCH 01/57] fix(session): recognize compression continuation across sub-second timestamp overlap _is_continuation_session() required float(child.started_at) >= float(parent.ended_at). A compression/cli_close handoff can persist the continuation row a few ms BEFORE the parent's ended_at lands (write-order race), so the strict comparison returned False despite every other piece of evidence (direct parent_session_id link, same source, end_reason=compression/cli_close, non-fork). The backend then labeled the continuation as relationship_type=child_session, causing duplicate same-title sidebar rows and spurious child-session entries (#6931). Allow a small tolerance (2s) for handoff-consistent timing; beyond it, an exact title match on the parent/child rows still confirms the same handoff. Fork, cross-source, and non-compression/cli_close guards are preserved, and materially overlapping children remain separate child sessions. Adds API-level regression tests (sub-second overlap forms one lineage; material overlap stays a child session; same-title evidence beyond tolerance) plus focused unit coverage of the classifier. Closes #6931 --- api/agent_sessions.py | 26 +++- tests/test_session_lineage_metadata_api.py | 159 +++++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) diff --git a/api/agent_sessions.py b/api/agent_sessions.py index c0a5078f..927f7063 100644 --- a/api/agent_sessions.py +++ b/api/agent_sessions.py @@ -53,6 +53,15 @@ MESSAGING_SOURCES = { CLI_MIN_UNTITLED_MESSAGE_COUNT = 6 CLI_MIN_UNTITLED_USER_MESSAGE_COUNT = 2 +# Sub-second scheduling/write-order races during a compression/cli_close +# handoff can persist the continuation row with ``started_at`` a few +# milliseconds BEFORE the parent's ``ended_at`` lands (#6931). The tolerance +# below lets those rows still classify as the next segment; the fork / +# cross-source / end_reason guards in ``_is_continuation_session`` already +# rule out unrelated rows, so a small window cannot collapse genuinely +# concurrently started children. +CONTINUATION_STARTED_AT_TOLERANCE_SECONDS = 2.0 + SOURCE_LABELS = { 'acp': 'ACP', 'api_server': 'API', @@ -312,7 +321,8 @@ def _is_continuation_session(parent: dict | None, child: dict | None) -> bool: by ``hermes -c`` also records a new child session; for sidebar projection it should continue the same visible conversation rather than becoming a separate child-session row. Plain parent/child links that started before the - parent's ended boundary remain child sessions. + parent's ended boundary remain child sessions. A small scheduling/write-order + overlap between the handoff boundary timestamps is tolerated (#6931). Do not collapse lineage across raw sources. A WebUI session that continues from a Telegram/CLI/etc. parent must remain visible as its own surface-owned @@ -336,9 +346,21 @@ def _is_continuation_session(parent: dict | None, child: dict | None) -> bool: # continuations when no boundary timestamp is available. return True try: - return float(child.get('started_at') or 0) >= float(ended_at) + child_started = float(child.get('started_at') or 0) + parent_ended = float(ended_at) except (TypeError, ValueError): return False + if child_started >= parent_ended - CONTINUATION_STARTED_AT_TOLERANCE_SECONDS: + return True + # Timestamps disagree beyond the tolerance. Independent evidence can still + # identify the same handoff: a compression/cli_close continuation carries + # the parent's conversation title verbatim (the #6931 rows had identical + # titles), while a genuinely new or forked session gets its own + # auto-numbered/user title. Requiring an exact title match keeps unrelated + # same-source children separate even when they overlap the parent. + parent_title = str(parent.get('title') or '').strip() + child_title = str(child.get('title') or '').strip() + return bool(parent_title and child_title and parent_title == child_title) def _continuation_root_id(rows_by_id: dict[str, dict], session_id: str | None) -> str | None: diff --git a/tests/test_session_lineage_metadata_api.py b/tests/test_session_lineage_metadata_api.py index 22635e57..f79848ce 100644 --- a/tests/test_session_lineage_metadata_api.py +++ b/tests/test_session_lineage_metadata_api.py @@ -693,3 +693,162 @@ def test_sessions_route_preserves_visible_child_lineage_when_archived_parent_fil ] finally: conn.close() + + +def test_compression_continuation_tolerates_sub_second_timestamp_overlap(_isolate): + """#6931: a compression continuation recorded a few ms BEFORE the parent's + ended_at (write-order race) is still one lineage in /api/sessions.""" + conn = _ensure_state_db(_isolate) + t0 = time.time() - 100 + try: + _save_webui_session("lineage_race_root", title="Shared conversation", updated_at=t0) + _save_webui_session("lineage_race_tip", title="Shared conversation", updated_at=t0 + 10) + _insert_state_row( + conn, + "lineage_race_root", + started_at=t0, + ended_at=t0 + 5, + end_reason="compression", + ) + # child.started_at lands 0.1s BEFORE parent.ended_at — the #6931 race. + _insert_state_row( + conn, + "lineage_race_tip", + parent="lineage_race_root", + started_at=t0 + 5 - 0.1, + ) + + rows = {row["session_id"]: row for row in all_sessions()} + + tip = rows["lineage_race_tip"] + assert tip.get("parent_session_id") == "lineage_race_root" + assert tip.get("_lineage_root_id") == "lineage_race_root" + assert tip.get("_lineage_tip_id") == "lineage_race_tip" + assert tip.get("_compression_segment_count") == 2 + assert tip.get("relationship_type") != "child_session" + finally: + conn.close() + + +def test_materially_overlapping_compression_child_stays_child_session(_isolate): + """#6931: a child that started long before the compression parent ended + (beyond tolerance, no matching title) must remain a separate child.""" + conn = _ensure_state_db(_isolate) + t0 = time.time() - 100 + try: + _save_webui_session("lineage_overlap_root", title="Root title", updated_at=t0) + _save_webui_session("lineage_overlap_child", title="Child title", updated_at=t0 + 10) + _insert_state_row( + conn, + "lineage_overlap_root", + started_at=t0, + ended_at=t0 + 60, + end_reason="compression", + ) + # child started 30s before the parent ended — a genuine concurrent child. + _insert_state_row( + conn, + "lineage_overlap_child", + parent="lineage_overlap_root", + started_at=t0 + 30, + ) + + rows = {row["session_id"]: row for row in all_sessions()} + + child = rows["lineage_overlap_child"] + assert child.get("relationship_type") == "child_session" + assert child.get("parent_session_id") == "lineage_overlap_root" + assert "_lineage_root_id" not in child + assert "_compression_segment_count" not in child + finally: + conn.close() + + +def test_same_title_compression_child_recognized_beyond_timestamp_tolerance(_isolate): + """#6931: a direct same-source child with the parent's exact title is still + a continuation even when the boundary timestamps disagree beyond the + tolerance — timestamp is not the only gate.""" + conn = _ensure_state_db(_isolate) + t0 = time.time() - 100 + try: + _save_webui_session("lineage_title_root", title="Shared conversation", updated_at=t0) + _save_webui_session("lineage_title_tip", title="Shared conversation", updated_at=t0 + 10) + _insert_state_row( + conn, + "lineage_title_root", + title="Shared conversation", + started_at=t0, + ended_at=t0 + 60, + end_reason="compression", + ) + # child started 30s before the parent ended but carries the exact title. + _insert_state_row( + conn, + "lineage_title_tip", + title="Shared conversation", + parent="lineage_title_root", + started_at=t0 + 30, + ) + + rows = {row["session_id"]: row for row in all_sessions()} + + tip = rows["lineage_title_tip"] + assert tip.get("_lineage_root_id") == "lineage_title_root" + assert tip.get("_lineage_tip_id") == "lineage_title_tip" + assert tip.get("_compression_segment_count") == 2 + assert tip.get("relationship_type") != "child_session" + finally: + conn.close() + + +def test_continuation_classification_timestamp_tolerance_and_guards(): + """#6931 focused unit coverage of _is_continuation_session: tolerance, + title evidence fallback, and preserved fork/cross-source/end_reason guards.""" + from api.agent_sessions import _is_continuation_session + + def make_parent(**over): + row = { + 'source': 'webui', + 'end_reason': 'compression', + 'ended_at': 1000.0, + 'title': 'Shared conversation', + } + row.update(over) + return row + + def make_child(**over): + row = { + 'source': 'webui', + 'started_at': 1000.05, + 'title': 'Shared conversation', + } + row.update(over) + return row + + # Normal non-overlapping ordering: continuation. + assert _is_continuation_session(make_parent(), make_child()) + # Sub-second overlap (observed -0.06..-0.07s in #6931): continuation. + assert _is_continuation_session(make_parent(), make_child(started_at=999.95)) + # Overlap inside the 2s tolerance: continuation. + assert _is_continuation_session(make_parent(), make_child(started_at=998.5)) + # Overlap beyond tolerance but exact title evidence: continuation. + assert _is_continuation_session(make_parent(), make_child(started_at=950.0)) + # Overlap beyond tolerance with a different title: separate child. + assert not _is_continuation_session( + make_parent(), make_child(started_at=950.0, title='Another conversation') + ) + # Fork guard holds regardless of timing. + assert not _is_continuation_session( + make_parent(), make_child(started_at=999.95, session_source='fork') + ) + # Cross-source guard holds regardless of timing. + assert not _is_continuation_session( + make_parent(), make_child(started_at=999.95, source='telegram') + ) + # Non-compression/cli_close parents never continue. + assert not _is_continuation_session( + make_parent(end_reason='user_stop'), make_child(started_at=999.95) + ) + # Missing/unparsable boundary timestamps degrade to False (no crash). + assert not _is_continuation_session(make_parent(ended_at='not-a-number'), make_child()) + assert not _is_continuation_session(make_parent(), make_child(started_at='not-a-number')) From 6f0028cfa1ca565a69d9c5945b1b7bde347e0a56 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 14 Aug 2026 17:26:05 -0300 Subject: [PATCH 02/57] fix(session): exclude model_config branches before tolerance; drop title fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-gate #7021: two silent sidebar regressions on the #6931 continuation tolerance. 1. MUST-FIX 1 — branch marker in model_config. The session_source=='fork' guard only covers WebUI-created forks (/api/session/branch). Explicit Agent branches/delegates carry no session_source: they are marked in the model_config JSON column (_branched_from / _delegate_from). The caller queries did not select model_config at all, so an Agent branch starting inside the 2s tolerance window of its parent's compression passed the widened check and vanished from the sidebar. Fix: select model_config in the classifier's caller queries (get_cli_sessions, lineage report, lineage metadata) and reject a child whose _branched_from / _delegate_from points at the parent being classified, BEFORE the tolerance is applied. The marker must reference this parent: compression continuations inherit the rotated agent's model_config verbatim (mirrors hermes-agent _NON_CONTINUATION_CHILD_FILTER_SQL). 2. MUST-FIX 2 — unbounded title fallback removed. Titles are user-controlled and non-unique, so parent_title == child_title beyond the tolerance window collapsed independent same-title branches arbitrarily far apart. The fallback is gone entirely; only the bounded 2s early-side tolerance (CONTINUATION_STARTED_AT_TOLERANCE_SECONDS) remains, after the reliable non-continuation guards. Tests: same-title independent child outside tolerance stays visible (was collapsed); _branched_from/_delegate_from children inside the tolerance stay visible (reviewer's adversarial probe); unit coverage for marker parsing (JSON string, dict, unparsable) and marker-points-at-other-session preservation. All 4 new tests are RED on the previous head and GREEN here. Closes #6931 --- api/agent_sessions.py | 77 ++++++-- ...t_issue5455_listing_readonly_connection.py | 71 ++++++++ tests/test_session_lineage_metadata_api.py | 165 +++++++++++++++--- 3 files changed, 279 insertions(+), 34 deletions(-) diff --git a/api/agent_sessions.py b/api/agent_sessions.py index 927f7063..76a47411 100644 --- a/api/agent_sessions.py +++ b/api/agent_sessions.py @@ -1,4 +1,5 @@ """Shared helpers for reading Hermes Agent sessions from state.db.""" +import json import logging import sqlite3 from contextlib import closing @@ -57,9 +58,9 @@ CLI_MIN_UNTITLED_USER_MESSAGE_COUNT = 2 # handoff can persist the continuation row with ``started_at`` a few # milliseconds BEFORE the parent's ``ended_at`` lands (#6931). The tolerance # below lets those rows still classify as the next segment; the fork / -# cross-source / end_reason guards in ``_is_continuation_session`` already -# rule out unrelated rows, so a small window cannot collapse genuinely -# concurrently started children. +# model_config branch-marker / cross-source / end_reason guards in +# ``_is_continuation_session`` already rule out unrelated rows, so a small +# window cannot collapse genuinely concurrently started children. CONTINUATION_STARTED_AT_TOLERANCE_SECONDS = 2.0 SOURCE_LABELS = { @@ -314,6 +315,30 @@ def is_cli_session_row_visible(row: dict) -> bool: return _count_user_turns(row) >= CLI_MIN_UNTITLED_USER_MESSAGE_COUNT +def _branch_markers(row: dict | None) -> tuple[str | None, str | None]: + """Return ``(_branched_from, _delegate_from)`` from a row's ``model_config``. + + Hermes Agent stamps explicit branches and delegate/subagent runs in the + ``model_config`` JSON column (``_branched_from`` / ``_delegate_from``); + ``session_source='fork'`` alone only covers WebUI-created forks. Missing, + empty, or unparsable ``model_config`` degrades to ``(None, None)``. + """ + if not row: + return None, None + raw = row.get('model_config') + if isinstance(raw, dict): + config = raw + elif isinstance(raw, str) and raw.strip(): + try: + parsed = json.loads(raw) + except (TypeError, ValueError): + parsed = None + config = parsed if isinstance(parsed, dict) else {} + else: + config = {} + return config.get('_branched_from'), config.get('_delegate_from') + + def _is_continuation_session(parent: dict | None, child: dict | None) -> bool: """Return True when ``child`` is the next segment of the same conversation. @@ -328,11 +353,29 @@ def _is_continuation_session(parent: dict | None, child: dict | None) -> bool: from a Telegram/CLI/etc. parent must remain visible as its own surface-owned conversation; otherwise the tip inherits the root's title/source metadata and can disappear under messaging/sidebar policies. + + Explicit branches/delegates never continue: a child whose ``model_config`` + ``_branched_from`` / ``_delegate_from`` points at this parent is a fork of + the conversation and must stay visible. Beyond the bounded tolerance window + there is no reliable continuation signal — titles are user-controlled and + non-unique, so they are never used to widen the window (#7021 re-gate). """ if not parent or not child: return False if str(child.get('session_source') or '').strip().lower() == 'fork': return False + # Real Agent branches/delegates are marked in model_config (not + # session_source, which only WebUI-created forks carry). The marker must + # reference THIS parent: compression continuations inherit the rotated + # agent's model_config verbatim, so presence alone (a delegate's + # continuation still carries the delegate's own ``_delegate_from``) would + # misclassify real continuations. + child_branched_from, child_delegate_from = _branch_markers(child) + parent_id = parent.get('id') + if parent_id and ( + child_branched_from == parent_id or child_delegate_from == parent_id + ): + return False parent_source = str(parent.get('source') or '').strip().lower() child_source = str(child.get('source') or '').strip().lower() if parent_source and child_source and parent_source != child_source: @@ -352,15 +395,10 @@ def _is_continuation_session(parent: dict | None, child: dict | None) -> bool: return False if child_started >= parent_ended - CONTINUATION_STARTED_AT_TOLERANCE_SECONDS: return True - # Timestamps disagree beyond the tolerance. Independent evidence can still - # identify the same handoff: a compression/cli_close continuation carries - # the parent's conversation title verbatim (the #6931 rows had identical - # titles), while a genuinely new or forked session gets its own - # auto-numbered/user title. Requiring an exact title match keeps unrelated - # same-source children separate even when they overlap the parent. - parent_title = str(parent.get('title') or '').strip() - child_title = str(child.get('title') or '').strip() - return bool(parent_title and child_title and parent_title == child_title) + # Beyond the bounded early-side window the child is a genuine concurrent + # session — an exact title match is not evidence (titles are user-controlled + # and non-unique, so it cannot extend the tolerance). + return False def _continuation_root_id(rows_by_id: dict[str, dict], session_id: str | None) -> str | None: @@ -594,6 +632,7 @@ def read_importable_agent_session_rows( parent_expr = _optional_col('parent_session_id', session_cols) session_source_expr = _optional_col('session_source', session_cols) + model_config_expr = _optional_col('model_config', session_cols) ended_expr = _optional_col('ended_at', session_cols) end_reason_expr = _optional_col('end_reason', session_cols) user_id_expr = _optional_col('user_id', session_cols) @@ -715,6 +754,7 @@ def read_importable_agent_session_rows( SELECT s.id, s.title, s.model, s.message_count, s.started_at, s.source, {session_source_expr}, + {model_config_expr}, {user_id_expr}, {chat_id_expr}, {chat_type_expr}, @@ -797,6 +837,11 @@ def read_importable_agent_session_rows( params, ) projected = _project_agent_session_rows([dict(row) for row in cur.fetchall()]) + # model_config is selected only so the continuation classifier can + # read the real _branched_from/_delegate_from markers; it is internal + # agent state and must not leak into the /api/sessions response. + for row in projected: + row.pop('model_config', None) projected = [_with_normalized_source(row) for row in projected] projected = [row for row in projected if is_cli_session_row_visible(row)] if limit is None: @@ -899,6 +944,7 @@ def read_session_lineage_report(db_path: Path, session_id: str | None, max_hops: source_expr = _optional_col('source', session_cols) session_source_expr = _optional_col('session_source', session_cols) + model_config_expr = _optional_col('model_config', session_cols) title_expr = _optional_col('title', session_cols) started_expr = _optional_col('started_at', session_cols, '0') ended_expr = _optional_col('ended_at', session_cols) @@ -913,6 +959,7 @@ def read_session_lineage_report(db_path: Path, session_id: str | None, max_hops: SELECT s.id, {source_expr}, {session_source_expr}, + {model_config_expr}, {title_expr}, {started_expr}, {parent_expr}, @@ -959,6 +1006,7 @@ def read_session_lineage_report(db_path: Path, session_id: str | None, max_hops: SELECT s.id, {source_expr}, {session_source_expr}, + {model_config_expr}, {title_expr}, {started_expr}, {parent_expr}, @@ -1036,6 +1084,7 @@ def read_session_lineage_metadata(db_path: Path, session_ids: list[str] | set[st return {} session_source_expr = _optional_col('session_source', session_cols) source_expr = _optional_col('source', session_cols) + model_config_expr = _optional_col('model_config', session_cols) message_count_expr = _optional_col('message_count', session_cols, '0') # Scoped fetch via PRIMARY KEY + idx_sessions_parent rather than a # full table scan. The sessions table grows unbounded over time @@ -1072,7 +1121,7 @@ def read_session_lineage_metadata(db_path: Path, session_ids: list[str] | set[st placeholders = ','.join('?' * len(chunk)) cur.execute( f""" - SELECT s.id, {source_expr}, {session_source_expr}, s.title, s.started_at, s.parent_session_id, s.ended_at, s.end_reason, {message_count_expr} + SELECT s.id, {source_expr}, {session_source_expr}, {model_config_expr}, s.title, s.started_at, s.parent_session_id, s.ended_at, s.end_reason, {message_count_expr} FROM sessions s WHERE s.id IN ({placeholders}) """, @@ -1101,7 +1150,7 @@ def read_session_lineage_metadata(db_path: Path, session_ids: list[str] | set[st placeholders = ','.join('?' * len(chunk)) cur.execute( f""" - SELECT s.id, {source_expr}, {session_source_expr}, s.title, s.started_at, s.parent_session_id, s.ended_at, s.end_reason, {message_count_expr} + SELECT s.id, {source_expr}, {session_source_expr}, {model_config_expr}, s.title, s.started_at, s.parent_session_id, s.ended_at, s.end_reason, {message_count_expr} FROM sessions s WHERE s.parent_session_id IN ({placeholders}) """, diff --git a/tests/test_issue5455_listing_readonly_connection.py b/tests/test_issue5455_listing_readonly_connection.py index fc2cb263..9502fae1 100644 --- a/tests/test_issue5455_listing_readonly_connection.py +++ b/tests/test_issue5455_listing_readonly_connection.py @@ -74,6 +74,77 @@ def test_listing_opens_read_only_and_returns_rows(tmp_path, monkeypatch): assert "mode=ro" in calls[0]["target"] +def test_listing_keeps_model_config_branch_visible_and_never_leaks_it(tmp_path, monkeypatch): + """#7021 re-gate: a CLI/agent branch whose _branched_from marker lives in + model_config must stay visible even when it starts inside the 2s + compression tolerance window, and model_config must not leak into the + returned rows.""" + db = tmp_path / "state.db" + conn = sqlite3.connect(str(db)) + conn.execute( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, model TEXT, message_count INTEGER, + started_at REAL, source TEXT, session_source TEXT, + parent_session_id TEXT, ended_at REAL, end_reason TEXT, + model_config TEXT + ) + """ + ) + conn.execute( + """ + INSERT INTO sessions + (id, title, model, message_count, started_at, source, session_source, + parent_session_id, ended_at, end_reason, model_config) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + "cli-parent", "Parent conversation", "gpt", 0, 1000.0, "cli", "cli", + None, 1100.0, "compression", None, + ), + ) + # An explicit Agent branch starting 1.5s BEFORE the parent's compression + # ended_at. No session_source — the fork identity lives in model_config. + conn.execute( + """ + INSERT INTO sessions + (id, title, model, message_count, started_at, source, session_source, + parent_session_id, ended_at, end_reason, model_config) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + "cli-branch", "Branch conversation", "gpt", 0, 1098.5, "cli", "cli", + "cli-parent", None, None, '{"_branched_from": "cli-parent"}', + ), + ) + conn.execute( + "CREATE TABLE messages (id INTEGER PRIMARY KEY, session_id TEXT, role TEXT, timestamp REAL)" + ) + conn.executemany( + "INSERT INTO messages (session_id, role, timestamp) VALUES (?,?,?)", + [ + ("cli-parent", "user", 1001.0), + ("cli-parent", "assistant", 1002.0), + ("cli-branch", "user", 1099.0), + ("cli-branch", "assistant", 1099.5), + ], + ) + conn.commit() + conn.close() + _record_connects(monkeypatch) + + out = read_importable_agent_session_rows(db, exclude_sources=None) + out_ids = {r["id"] for r in out} + + # The branch survives the tolerance window: it is NOT collapsed into the + # parent lineage. + assert "cli-branch" in out_ids + assert "cli-parent" in out_ids + # model_config is internal agent state — never exposed on returned rows. + for row in out: + assert "model_config" not in row + + def test_listing_read_only_uri_encodes_special_path_chars(tmp_path, monkeypatch): db_dir = tmp_path / "state dir #1" db_dir.mkdir() diff --git a/tests/test_session_lineage_metadata_api.py b/tests/test_session_lineage_metadata_api.py index f79848ce..4d3c9c8c 100644 --- a/tests/test_session_lineage_metadata_api.py +++ b/tests/test_session_lineage_metadata_api.py @@ -1,5 +1,6 @@ """Regression tests for /api/sessions lineage metadata used by sidebar collapse.""" +import json import sqlite3 import time @@ -51,7 +52,8 @@ def _ensure_state_db(path): message_count INTEGER DEFAULT 0, parent_session_id TEXT, ended_at REAL, - end_reason TEXT + end_reason TEXT, + model_config TEXT ); """ ) @@ -79,14 +81,14 @@ def _insert_state_message(conn, sid, *, role, content, timestamp): conn.commit() -def _insert_state_row(conn, sid, *, title=None, parent=None, ended_at=None, end_reason=None, started_at=None, source='webui', session_source=None): +def _insert_state_row(conn, sid, *, title=None, parent=None, ended_at=None, end_reason=None, started_at=None, source='webui', session_source=None, model_config=None): conn.execute( """ INSERT INTO sessions - (id, source, session_source, title, model, started_at, message_count, parent_session_id, ended_at, end_reason) - VALUES (?, ?, ?, ?, 'openai/gpt-5', ?, 2, ?, ?, ?) + (id, source, session_source, title, model, started_at, message_count, parent_session_id, ended_at, end_reason, model_config) + VALUES (?, ?, ?, ?, 'openai/gpt-5', ?, 2, ?, ?, ?, ?) """, - (sid, source, session_source, title or sid, started_at or time.time(), parent, ended_at, end_reason), + (sid, source, session_source, title or sid, started_at or time.time(), parent, ended_at, end_reason, model_config), ) conn.commit() @@ -764,10 +766,11 @@ def test_materially_overlapping_compression_child_stays_child_session(_isolate): conn.close() -def test_same_title_compression_child_recognized_beyond_timestamp_tolerance(_isolate): - """#6931: a direct same-source child with the parent's exact title is still - a continuation even when the boundary timestamps disagree beyond the - tolerance — timestamp is not the only gate.""" +def test_same_title_independent_child_outside_tolerance_stays_visible(_isolate): + """#7021 re-gate: a same-title independent child started well outside the + tolerance window must NOT collapse on the title match — titles are + user-controlled and non-unique, so the only continuation evidence is the + bounded early-side timestamp window.""" conn = _ensure_state_db(_isolate) t0 = time.time() - 100 try: @@ -781,33 +784,110 @@ def test_same_title_compression_child_recognized_beyond_timestamp_tolerance(_iso ended_at=t0 + 60, end_reason="compression", ) - # child started 30s before the parent ended but carries the exact title. + # Same title, but started 50s before the parent ended — far outside any + # handoff race window. Must remain a visible child session. _insert_state_row( conn, "lineage_title_tip", title="Shared conversation", parent="lineage_title_root", - started_at=t0 + 30, + started_at=t0 + 10, ) rows = {row["session_id"]: row for row in all_sessions()} tip = rows["lineage_title_tip"] - assert tip.get("_lineage_root_id") == "lineage_title_root" - assert tip.get("_lineage_tip_id") == "lineage_title_tip" - assert tip.get("_compression_segment_count") == 2 - assert tip.get("relationship_type") != "child_session" + assert tip.get("relationship_type") == "child_session" + assert tip.get("parent_session_id") == "lineage_title_root" + assert "_lineage_root_id" not in tip + assert "_compression_segment_count" not in tip + finally: + conn.close() + + +def test_model_config_branched_from_child_stays_visible_within_tolerance(_isolate): + """#7021 re-gate: an explicit Agent branch is marked in + model_config._branched_from (not session_source). Even when it starts + inside the 2s tolerance window of the parent's compression, the real + marker must keep it visible instead of collapsing it into the lineage.""" + conn = _ensure_state_db(_isolate) + t0 = time.time() - 100 + try: + _save_webui_session("lineage_branch_root", title="Shared conversation", updated_at=t0) + _save_webui_session("lineage_branch_tip", title="Shared conversation", updated_at=t0 + 10) + _insert_state_row( + conn, + "lineage_branch_root", + started_at=t0, + ended_at=t0 + 5, + end_reason="compression", + ) + # The reviewer's adversarial probe: an Agent branch starting 1.5s + # BEFORE the parent's compression ended_at. No session_source — the + # fork identity lives in model_config._branched_from. + _insert_state_row( + conn, + "lineage_branch_tip", + parent="lineage_branch_root", + started_at=t0 + 5 - 1.5, + model_config=json.dumps({"_branched_from": "lineage_branch_root"}), + ) + + rows = {row["session_id"]: row for row in all_sessions()} + + tip = rows["lineage_branch_tip"] + assert tip.get("relationship_type") == "child_session" + assert tip.get("parent_session_id") == "lineage_branch_root" + assert "_lineage_root_id" not in tip + assert "_compression_segment_count" not in tip + finally: + conn.close() + + +def test_model_config_delegate_from_child_stays_visible_within_tolerance(_isolate): + """#7021 re-gate: delegate/subagent runs are marked in + model_config._delegate_from. A delegate child starting inside the + tolerance window of the parent's compression must stay visible.""" + conn = _ensure_state_db(_isolate) + t0 = time.time() - 100 + try: + _save_webui_session("lineage_delegate_root", title="Shared conversation", updated_at=t0) + _save_webui_session("lineage_delegate_tip", title="Shared conversation", updated_at=t0 + 10) + _insert_state_row( + conn, + "lineage_delegate_root", + started_at=t0, + ended_at=t0 + 5, + end_reason="compression", + ) + _insert_state_row( + conn, + "lineage_delegate_tip", + parent="lineage_delegate_root", + started_at=t0 + 5 - 1.0, + model_config=json.dumps({"_delegate_from": "lineage_delegate_root"}), + ) + + rows = {row["session_id"]: row for row in all_sessions()} + + tip = rows["lineage_delegate_tip"] + assert tip.get("relationship_type") == "child_session" + assert tip.get("parent_session_id") == "lineage_delegate_root" + assert "_lineage_root_id" not in tip + assert "_compression_segment_count" not in tip finally: conn.close() def test_continuation_classification_timestamp_tolerance_and_guards(): - """#6931 focused unit coverage of _is_continuation_session: tolerance, - title evidence fallback, and preserved fork/cross-source/end_reason guards.""" + """#6931/#7021 focused unit coverage of _is_continuation_session: bounded + tolerance, model_config branch markers, and preserved fork/cross-source/ + end_reason guards.""" from api.agent_sessions import _is_continuation_session def make_parent(**over): row = { + 'id': 'parent-1', 'source': 'webui', 'end_reason': 'compression', 'ended_at': 1000.0, @@ -818,6 +898,7 @@ def test_continuation_classification_timestamp_tolerance_and_guards(): def make_child(**over): row = { + 'id': 'child-1', 'source': 'webui', 'started_at': 1000.05, 'title': 'Shared conversation', @@ -831,12 +912,56 @@ def test_continuation_classification_timestamp_tolerance_and_guards(): assert _is_continuation_session(make_parent(), make_child(started_at=999.95)) # Overlap inside the 2s tolerance: continuation. assert _is_continuation_session(make_parent(), make_child(started_at=998.5)) - # Overlap beyond tolerance but exact title evidence: continuation. - assert _is_continuation_session(make_parent(), make_child(started_at=950.0)) - # Overlap beyond tolerance with a different title: separate child. + # Overlap beyond tolerance: separate child, even with an exact title match + # (titles are user-controlled and non-unique — no title fallback). + assert not _is_continuation_session(make_parent(), make_child(started_at=950.0)) assert not _is_continuation_session( make_parent(), make_child(started_at=950.0, title='Another conversation') ) + # model_config._branched_from pointing at the parent: never a + # continuation, regardless of timing. + assert not _is_continuation_session( + make_parent(), + make_child( + started_at=999.95, + model_config=json.dumps({'_branched_from': 'parent-1'}), + ), + ) + assert not _is_continuation_session( + make_parent(), + make_child( + started_at=998.5, + model_config={'_branched_from': 'parent-1'}, + ), + ) + # model_config._delegate_from pointing at the parent: never a + # continuation, regardless of timing. + assert not _is_continuation_session( + make_parent(), + make_child( + started_at=999.95, + model_config=json.dumps({'_delegate_from': 'parent-1'}), + ), + ) + # A marker pointing at a DIFFERENT session does not disqualify: compression + # continuations inherit the rotated agent's model_config verbatim, so a + # delegate's continuation still carries the delegate's own marker. + assert _is_continuation_session( + make_parent(), + make_child( + started_at=999.95, + model_config=json.dumps({'_delegate_from': 'some-other-session'}), + ), + ) + # Unparsable model_config degrades to no markers (no crash, no match). + assert _is_continuation_session( + make_parent(), + make_child(started_at=999.95, model_config='not-json'), + ) + assert not _is_continuation_session( + make_parent(), + make_child(started_at=950.0, model_config='not-json'), + ) # Fork guard holds regardless of timing. assert not _is_continuation_session( make_parent(), make_child(started_at=999.95, session_source='fork') From 2e12075400dbff9d84ea515f1637d2ecd900ccae Mon Sep 17 00:00:00 2001 From: webtecnica Date: Mon, 17 Aug 2026 10:47:29 -0300 Subject: [PATCH 03/57] fix(session): apply branch-marker guard to transcript-stitcher lineage query (#7021 r2) The open/import transcript stitcher (get_state_db_session_messages with stitch_continuations=True) runs its own lineage SELECT that omitted session_source/model_config, so the shared _is_continuation_session guard added for the listing path never fired there: an explicit branch/delegate child (model_config._branched_from/_delegate_from) starting within the 2s tolerance window got its rows stitched into the parent transcript on open. Select both identity columns when the schema has them (NULL AS fallback for older state.db schemas), so the same guard applies in the stitch path. Tests (tests/test_session_lineage_metadata_api.py): - branched/delegate children inside the tolerance stay out of the parent transcript on open/import (fail before this fix, pass after); - old-schema rows without the identity columns still stitch genuine compression continuations (no crash, no behavior change). --- api/models.py | 25 ++- tests/test_session_lineage_metadata_api.py | 171 +++++++++++++++++++++ 2 files changed, 192 insertions(+), 4 deletions(-) diff --git a/api/models.py b/api/models.py index 2ad051b2..957eb519 100644 --- a/api/models.py +++ b/api/models.py @@ -8339,9 +8339,26 @@ def get_state_db_session_messages( cur.execute("PRAGMA table_info(sessions)") session_cols = {str(row['name']) for row in cur.fetchall()} if {'parent_session_id', 'end_reason', 'started_at', 'source'}.issubset(session_cols): + # session_source/model_config carry the fork/delegate boundary + # identity consumed by _is_continuation_session (#6931/#7021). + # Select them when present so the branch-marker guard applies + # to the stitch path too; older schemas degrade to NULL. + identity_cols = [] + if 'session_source' in session_cols: + identity_cols.append('session_source') + else: + identity_cols.append('NULL AS session_source') + if 'model_config' in session_cols: + identity_cols.append('model_config') + else: + identity_cols.append('NULL AS model_config') + lineage_select = ( + "id, source, started_at, parent_session_id, ended_at, end_reason" + + ", " + ", ".join(identity_cols) + ) cur.execute( - """ - SELECT id, source, started_at, parent_session_id, ended_at, end_reason + f""" + SELECT {lineage_select} FROM sessions WHERE id = ? """, @@ -8359,8 +8376,8 @@ def get_state_db_session_messages( if not parent_id or parent_id in seen: break cur.execute( - """ - SELECT id, source, started_at, parent_session_id, ended_at, end_reason + f""" + SELECT {lineage_select} FROM sessions WHERE id = ? """, diff --git a/tests/test_session_lineage_metadata_api.py b/tests/test_session_lineage_metadata_api.py index 4d3c9c8c..6e35bc1b 100644 --- a/tests/test_session_lineage_metadata_api.py +++ b/tests/test_session_lineage_metadata_api.py @@ -64,6 +64,7 @@ def _ensure_messages_table(conn): conn.execute( """ CREATE TABLE messages ( + id INTEGER PRIMARY KEY, session_id TEXT, role TEXT, content TEXT, @@ -977,3 +978,173 @@ def test_continuation_classification_timestamp_tolerance_and_guards(): # Missing/unparsable boundary timestamps degrade to False (no crash). assert not _is_continuation_session(make_parent(ended_at='not-a-number'), make_child()) assert not _is_continuation_session(make_parent(), make_child(started_at='not-a-number')) + + +def test_state_db_stitch_keeps_branched_child_out_of_parent_transcript(_isolate): + """#7021 r2: the open/import transcript stitcher (get_state_db_session_messages + with stitch_continuations=True) must apply the same model_config branch-marker + guard as the listing classifier. A child whose model_config._branched_from / + _delegate_from points at the parent must NOT have its messages stitched into + the parent transcript even when it starts inside the tolerance window. + """ + conn = _ensure_state_db(_isolate) + _ensure_messages_table(conn) + t0 = time.time() - 100 + try: + # Compression parent whose child starts INSIDE the 2s tolerance window. + _insert_state_row( + conn, + "stitch_branch_parent", + started_at=t0, + ended_at=t0 + 5, + end_reason="compression", + ) + _insert_state_message( + conn, "stitch_branch_parent", role="user", content="parent turn", timestamp=t0 + 1 + ) + # Explicit Agent branch: fork identity lives in model_config, not + # session_source. Starting 1.5s before the parent's ended_at — inside + # the tolerance — it must still stay out of the parent's transcript. + _insert_state_row( + conn, + "stitch_branch_child", + parent="stitch_branch_parent", + started_at=t0 + 5 - 1.5, + model_config=json.dumps({"_branched_from": "stitch_branch_parent"}), + ) + _insert_state_message( + conn, "stitch_branch_child", role="user", content="branch turn", timestamp=t0 + 6 + ) + + msgs = models.get_state_db_session_messages( + "stitch_branch_child", stitch_continuations=True + ) + contents = [m["content"] for m in msgs] + assert "branch turn" in contents + assert "parent turn" not in contents + + # Control: a genuine compression continuation (no branch marker) starting + # inside the same window IS stitched into the parent transcript. + _insert_state_row( + conn, + "stitch_continuation_child", + parent="stitch_branch_parent", + started_at=t0 + 5 - 0.5, + ) + _insert_state_message( + conn, + "stitch_continuation_child", + role="user", + content="continuation turn", + timestamp=t0 + 7, + ) + msgs = models.get_state_db_session_messages( + "stitch_continuation_child", stitch_continuations=True + ) + contents = [m["content"] for m in msgs] + assert "continuation turn" in contents + assert "parent turn" in contents + finally: + conn.close() + + +def test_state_db_stitch_keeps_delegate_child_out_of_parent_transcript(_isolate): + """#7021 r2: same guard for model_config._delegate_from (subagent runs) in + the open/import transcript stitcher.""" + conn = _ensure_state_db(_isolate) + _ensure_messages_table(conn) + t0 = time.time() - 100 + try: + _insert_state_row( + conn, + "stitch_delegate_parent", + started_at=t0, + ended_at=t0 + 5, + end_reason="compression", + ) + _insert_state_message( + conn, "stitch_delegate_parent", role="user", content="parent turn", timestamp=t0 + 1 + ) + _insert_state_row( + conn, + "stitch_delegate_child", + parent="stitch_delegate_parent", + started_at=t0 + 5 - 1.0, + model_config=json.dumps({"_delegate_from": "stitch_delegate_parent"}), + ) + _insert_state_message( + conn, "stitch_delegate_child", role="user", content="delegate turn", timestamp=t0 + 6 + ) + + msgs = models.get_state_db_session_messages( + "stitch_delegate_child", stitch_continuations=True + ) + contents = [m["content"] for m in msgs] + assert "delegate turn" in contents + assert "parent turn" not in contents + finally: + conn.close() + + +def test_state_db_stitch_old_schema_without_identity_columns_still_stitches(_isolate): + """#7021 r2: older state.db schemas lacking session_source/model_config degrade + to NULL and keep the pre-fix behavior — a genuine compression continuation + within the tolerance is still stitched, without crashing.""" + conn = sqlite3.connect(str(_isolate)) + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + source TEXT, + title TEXT, + started_at REAL NOT NULL, + message_count INTEGER DEFAULT 0, + parent_session_id TEXT, + ended_at REAL, + end_reason TEXT + ); + """ + ) + _ensure_messages_table(conn) + t0 = time.time() - 100 + try: + conn.execute( + """ + INSERT INTO sessions + (id, source, title, started_at, message_count, parent_session_id, ended_at, end_reason) + VALUES (?, ?, ?, ?, 2, ?, ?, ?) + """, + ( + "stitch_old_parent", + "webui", + "stitch_old_parent", + t0, + None, + t0 + 5, + "compression", + ), + ) + conn.execute( + """ + INSERT INTO sessions + (id, source, title, started_at, message_count, parent_session_id, ended_at, end_reason) + VALUES (?, ?, ?, ?, 2, ?, NULL, NULL) + """, + ("stitch_old_child", "webui", "stitch_old_child", t0 + 4.5, "stitch_old_parent"), + ) + conn.commit() + _insert_state_message( + conn, "stitch_old_parent", role="user", content="parent turn", timestamp=t0 + 1 + ) + _insert_state_message( + conn, "stitch_old_child", role="user", content="old child turn", timestamp=t0 + 6 + ) + + msgs = models.get_state_db_session_messages( + "stitch_old_child", stitch_continuations=True + ) + contents = [m["content"] for m in msgs] + assert "old child turn" in contents + assert "parent turn" in contents + finally: + conn.close() From 6f85d6e7807466758c9eda8902bc786a44a2d77e Mon Sep 17 00:00:00 2001 From: Brent Atchison Date: Fri, 4 Sep 2026 11:12:56 -0500 Subject: [PATCH 04/57] perf(sessions): skip redundant tool replay after scene restore --- .github/workflows/conversation-lifecycle.yml | 6 + TESTING.md | 29 +++- static/sessions.js | 6 + tests/browser_reconnect_scene_redraw.py | 171 +++++++++++++++++++ tests/test_reconnect_scene_redraw.py | 82 +++++++++ tests/test_static_asset_resolver.py | 9 +- 6 files changed, 297 insertions(+), 6 deletions(-) create mode 100644 tests/browser_reconnect_scene_redraw.py create mode 100644 tests/test_reconnect_scene_redraw.py diff --git a/.github/workflows/conversation-lifecycle.yml b/.github/workflows/conversation-lifecycle.yml index e952d34f..a1898cdd 100644 --- a/.github/workflows/conversation-lifecycle.yml +++ b/.github/workflows/conversation-lifecycle.yml @@ -13,6 +13,7 @@ on: - 'server.py' - 'tests/browser_conversation_lifecycle.py' - 'tests/browser_historical_transcript_hydration.py' + - 'tests/browser_reconnect_scene_redraw.py' - 'requirements*.txt' - 'pyproject.toml' - '.github/workflows/conversation-lifecycle.yml' @@ -24,6 +25,7 @@ on: - 'server.py' - 'tests/browser_conversation_lifecycle.py' - 'tests/browser_historical_transcript_hydration.py' + - 'tests/browser_reconnect_scene_redraw.py' - 'requirements*.txt' - 'pyproject.toml' - '.github/workflows/conversation-lifecycle.yml' @@ -45,6 +47,9 @@ jobs: scenario: terminal-error - name: historical-transcript-hydration script: tests/browser_historical_transcript_hydration.py + - name: reconnect-scene-redraw + script: tests/browser_reconnect_scene_redraw.py + browsers: chromium steps: - uses: actions/checkout@v4 @@ -69,6 +74,7 @@ jobs: LIFECYCLE_ARTIFACT_DIR: lifecycle-artifacts HISTORICAL_HYDRATION_ARTIFACT_DIR: lifecycle-artifacts LIFECYCLE_SCENARIO: ${{ matrix.scenario }} + BROWSERS: ${{ matrix.browsers }} run: python ${{ matrix.script }} - name: Run settlement frame proof diff --git a/TESTING.md b/TESTING.md index 6b2f2862..d8fd0ff5 100644 --- a/TESTING.md +++ b/TESTING.md @@ -137,11 +137,32 @@ HISTORICAL_HYDRATION_TEST_BITE=break-tool-link \ ``` The dedicated `Conversation lifecycle (informational)` workflow runs the current -proof rows (`normal`, `terminal-error`, and `historical-transcript-hydration`) and -stays non-blocking while the public +proof rows (`normal`, `terminal-error`, `historical-transcript-hydration`, and the +Chromium `reconnect-scene-redraw` matrix) and stays non-blocking while the public matrix expands to additional behavior rows. The maintainer's private QA harness -remains broader; later public slices will add session switching, reconnect/replay, -cancellation, compression, and recovery. +remains broader; later public slices will add cancellation, compression, and +recovery coverage. + +### Active-session reconnect redraw gate + +`tests/browser_reconnect_scene_redraw.py` is an opt-in deterministic browser +gate for reconnecting to a running session with a large Anchor activity scene. +It uses an isolated temporary server/home and fixture SSE events—no agent, +provider credentials, or production state. By default it checks Chromium and +WebKit, desktop and 390px viewports, and all three activity display modes: + +```bash +pip install playwright +python -m playwright install chromium webkit +python tests/browser_reconnect_scene_redraw.py +``` + +`TOOL_COUNT`, `BROWSERS`, and `MODES` narrow the matrix. `MEASURE_BASELINE=1` +records an unoptimized baseline without enforcing the redraw ceiling; +`METRICS_FILE` saves JSON metrics and `SCREENSHOT_DIR` saves generated-fixture +screenshots. This gate tests page reconstruction, journal-cursor resume, and +subsequent fixture SSE updates. It does not test a real provider/network or PWA +service-worker cache behavior. ### Streaming reader intent diff --git a/static/sessions.js b/static/sessions.js index b7a59359..60daf168 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -2165,6 +2165,12 @@ async function loadSession(sid){ S.activeStreamId=activeStreamId; const liveToolReplayId=(tc)=>String(tc&&(tc.tid||tc.id||tc.tool_call_id||tc.tool_use_id||tc.call_id||'')||'').trim(); const replayPersistedLiveToolCards=(opts)=>{ + // The journal-backed Anchor scene is authoritative through its resume + // cursor; newer rows arrive through the reattached SSE stream. Replaying + // the older INFLIGHT tool cache after a successful scene restore would + // redraw all N rows N times. Keep the #3707 replay only for legacy HTML + // restoration or a failed/unavailable scene render. + if(restoredAnchorScene) return; const liveToolCalls=Array.isArray(S.toolCalls) ? S.toolCalls : (Array.isArray(INFLIGHT[sid]&&INFLIGHT[sid].toolCalls)?INFLIGHT[sid].toolCalls:[]); diff --git a/tests/browser_reconnect_scene_redraw.py b/tests/browser_reconnect_scene_redraw.py new file mode 100644 index 00000000..8bc5118e --- /dev/null +++ b/tests/browser_reconnect_scene_redraw.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +"""Real page/loadSession/renderers with deterministic session and SSE fixtures. + +No agent, provider, production state, or credentials. Tests DOM reconstruction, +not a real provider/network reconnect. +""" +import json +import os +import tempfile +import time +from pathlib import Path +from urllib.parse import parse_qs, urlsplit + +from playwright.sync_api import sync_playwright + +from browser_conversation_lifecycle import _start_webui_server, _terminate_process + +ROOT = Path(os.environ.get('WEBUI_TEST_ROOT', Path(__file__).resolve().parent.parent)) + + +def fixture(count): + tools = [dict(name='terminal', tid=f'tool-{i}', args={'command': f'printf result-{i}'}, + preview=f'result-{i}', snippet=f'result-{i}', done=True) for i in range(count)] + rows = [dict(row_id=f'tool:{t["tid"]}', local_id=t['tid'], role='tool', kind='tool_call', + source_event_type='tool_complete', status='completed', order_index=i, + tool=t, payload=t) for i, t in enumerate(tools)] + return dict(stream_id='run-fixture', last_seq=1000, last_event_id='run-fixture:1000', + messages=[], tool_calls=tools, last_assistant_text='', last_reasoning_text='', + anchor_activity_scene=dict(version='activity_scene_v1', + identity=dict(session_id='fixture', stream_id='run-fixture', run_id='run-fixture'), + activity_rows=rows)) + + +INIT = """ +// A controlling service worker bypasses Playwright's page route fixtures in +// WebKit. This test targets reconnect rendering, not PWA cache behavior. +if(window===window.top && 'serviceWorker' in navigator){ + navigator.serviceWorker.register=()=>Promise.reject(new Error('Disabled in reconnect harness')); +} +window.fixtureSources=[]; +class FixtureEventSource { + static OPEN=1; static CONNECTING=0; static CLOSED=2; + constructor(url){this.url=String(url);this.readyState=1;this.listeners={};window.fixtureSources.push(this);} + addEventListener(name,fn){(this.listeners[name]||=[]).push(fn);} + removeEventListener(){} + close(){this.readyState=2;} + emit(name,data,id){for(const fn of this.listeners[name]||[])fn({data:JSON.stringify(data),lastEventId:id||''});} +} +window.EventSource=FixtureEventSource; +""" + + +def session_route(session, sid, workspace): + def handle(route): + query = parse_qs(urlsplit(route.request.url).query) + requested = query.get('session_id', [''])[0] + data = session if requested == sid else dict( + session_id='idle-fixture', messages=[], message_count=0, + tool_calls=[], workspace=workspace) + route.fulfill(json={'session': data}) + return handle + + +def main(): + tool_count = int(os.environ.get('TOOL_COUNT', '470')) + snapshot = fixture(tool_count) + stream_id = snapshot['stream_id'] + sid = 'fixture' + with tempfile.TemporaryDirectory(prefix='webui-reconnect-') as temp: + state = Path(temp) + env = {k:os.environ[k] for k in ('PATH','SYSTEMROOT','TMPDIR') if k in os.environ} + env.update(HOME=temp, HERMES_HOME=temp, HERMES_BASE_HOME=temp, + HERMES_WEBUI_STATE_DIR=str(state/'webui'), HERMES_CONFIG_PATH=str(state/'config.yaml'), + HERMES_WEBUI_HOST='127.0.0.1', HERMES_WEBUI_SKIP_ONBOARDING='1', + HERMES_WEBUI_AGENT_DIR=str(state/'no-agent')) + proc, log, _, base = _start_webui_server(ROOT, env, state) + results = [] + try: + with sync_playwright() as pw: + for engine in os.environ.get('BROWSERS','chromium,webkit').split(','): + browser = getattr(pw,engine).launch(headless=True) + try: + for mode in os.environ.get('MODES','compact_worklog,transparent_stream,hide_all_activity').split(','): + for width in [1280,390]: + context = browser.new_context(viewport={'width':width,'height':844},bypass_csp=True) + context.add_init_script(INIT) + page = context.new_page() + + errors=[] + page.on('pageerror',lambda e, errors=errors:errors.append(str(e))) + session = dict(session_id=sid,title='Reconnect performance fixture',model='', + workspace=temp,messages=[],message_count=0,tool_calls=[], + active_stream_id=stream_id,pending_user_message='Inspect the fixture', + pending_started_at=1,runtime_journal_snapshot=snapshot) + page.route('**/api/session?*',session_route(session,sid,temp)) + page.route('**/api/chat/stream/status?*',lambda r:r.fulfill(json={'active':True})) + page.goto(base,wait_until='load') + # WebKit's wait_for_function uses eval, blocked by + # the app CSP even with bypass_csp. Inspector + # evaluation still works; wait on actual boot state. + deadline=time.monotonic()+30 + while not page.evaluate("typeof loadSession==='function' && S._bootReady === true"): + assert time.monotonic(){ + window._chatActivityDisplayMode=mode;window._showThinking=true; + window._simplifiedToolCalling=true; + window.redrawCount=0; + const render=window._renderLiveAnchorActivitySceneForStream; + window._renderLiveAnchorActivitySceneForStream=function(...args){window.redrawCount++;return render(...args);}; + }""",mode) + measured=page.evaluate("""async sid=>{ + const t=performance.now();await loadSession(sid); + const loadMs=performance.now()-t; + const deadline=performance.now()+5000; + while(!fixtureSources.some(s=>s.url.includes('api/chat/stream?'))&&performance.now()setTimeout(r,10)); + } + return {ms:loadMs,redraws:window.redrawCount, + tools:document.querySelectorAll('#liveAssistantTurn [data-anchor-row-role="tool"]').length, + busy:S.busy, + sources:fixtureSources.filter(s=>s.url.includes('api/chat/stream?')).map(s=>s.url)}; + }""",sid) + result=dict(engine=engine,mode=mode,width=width, + **{k:v for k,v in measured.items() if k!='sources'}) + print(json.dumps(result),flush=True) + results.append(result) + if not os.environ.get('MEASURE_BASELINE'): + assert measured['redraws']<=2,result + expected=0 if mode=='hide_all_activity' else tool_count + assert measured['tools']==expected,result + assert measured['busy'],result + assert measured['sources'], (result, errors) + cursor=parse_qs(urlsplit(measured['sources'][-1]).query) + assert cursor['after_seq']==[str(snapshot['last_seq'])], (result,errors) + if mode!='hide_all_activity': + # Subsequent real SSE handler updates still paint the existing owner. + updated=page.evaluate("""({sid,stream})=>{ + const source=fixtureSources.findLast(s=>s.url.includes('api/chat/stream?')&&s.readyState===1); + if(!source)throw new Error('missing chat stream'); + source.emit('tool',{name:'terminal',tid:'after-reconnect',args:{command:'printf later'},preview:'later'},stream+':1001'); + source.emit('tool_complete',{name:'terminal',tid:'after-reconnect',preview:'LATER RESULT',duration:1},stream+':1002'); + return {rows:document.querySelectorAll('#liveAssistantTurn [data-anchor-row-role="tool"]').length, + result:document.querySelector('#liveAssistantTurn').textContent.includes('LATER RESULT')}; + }""",{'sid':sid,'stream':stream_id}) + assert updated=={'rows':expected+1,'result':True},updated + switched=page.evaluate("""async sid=>{ + await loadSession('idle-fixture'); + await loadSession(sid); + return {rows:document.querySelectorAll('#liveAssistantTurn [data-anchor-row-role="tool"]').length, + result:document.querySelector('#liveAssistantTurn').textContent.includes('LATER RESULT')}; + }""",sid) + assert switched==updated,switched + artifact=os.environ.get('SCREENSHOT_DIR') + if artifact: + Path(artifact).mkdir(parents=True,exist_ok=True) + page.screenshot(path=str(Path(artifact)/f'{engine}-{mode}-{width}.png')) + assert not errors,errors + context.close() + finally: + browser.close() + output=os.environ.get('METRICS_FILE') + if output: + Path(output).write_text(json.dumps(results,indent=2)) + finally: + _terminate_process(proc) + log.close() + + +if __name__=='__main__': + main() diff --git a/tests/test_reconnect_scene_redraw.py b/tests/test_reconnect_scene_redraw.py new file mode 100644 index 00000000..1c1f03e3 --- /dev/null +++ b/tests/test_reconnect_scene_redraw.py @@ -0,0 +1,82 @@ +"""Execute the production reconnect replay seam, not a source-string assertion. + +Browser coverage in browser_reconnect_scene_redraw.py exercises loadSession and +real DOM rendering; these cheap Node checks pin fallback and redraw counts. +""" +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +NODE = shutil.which('node') +pytestmark = pytest.mark.skipif(NODE is None, reason='Node.js is required for the reconnect replay probe') + + +def source_slice(text, start_marker, end_marker, path): + start = text.find(start_marker) + assert start >= 0, f'{path}: missing start marker {start_marker!r}' + end = text.find(end_marker, start) + assert end >= 0, f'{path}: missing end marker {end_marker!r}' + return text[start:end] + + +def replay_probe(restored_scene, count, skip_unkeyed=True, has_rows=True): + sessions = (ROOT / 'static/sessions.js').read_text() + ui = (ROOT / 'static/ui.js').read_text() + replay = source_slice( + sessions, 'const liveToolReplayId=(tc)=>', ' let didReconnect=false;', + 'static/sessions.js', + ) + append = source_slice( + ui, 'function appendLiveToolCard(', '\nfunction _findLatestLiveAssistantByBurst', + 'static/ui.js', + ) + script = r""" +const vm=require('node:vm'); +const opts=JSON.parse(process.argv[1]); +const tools=Array.from({length:opts.count},(_,i)=>({name:'terminal',tid:'tool-'+i,done:true})); +tools.push({name:'read_file',done:true}); +let redraws=0, appended=[]; +const sandbox={ + S:{session:{session_id:'session'},activeStreamId:'run',toolCalls:tools}, + sid:'session',activeStreamId:'run',INFLIGHT:{},restoredAnchorScene:opts.restored_scene, + document:{getElementById:()=>({querySelector:()=>opts.has_rows?{}:null})}, + isFinalAnswerOnlyMode:()=>false, + isLiveAnchorActivitySceneOwner:()=>true, + _renderLiveAnchorActivitySceneForStream:()=>{redraws++;return true;}, +}; +vm.createContext(sandbox); +vm.runInContext(APPEND,sandbox); +// The actual append function proves the amplification when a scene owns DOM. +// For legacy fallback, observe arguments at the append boundary instead. +if(!opts.restored_scene) sandbox.appendLiveToolCard=(tc)=>appended.push(tc.tid||'unkeyed'); +vm.runInContext(REPLAY+'\nreplayPersistedLiveToolCards('+JSON.stringify({skipUnkeyedRestoredDuplicates:opts.skip_unkeyed})+');',sandbox); +console.log(JSON.stringify({redraws,appended})); +""".replace('APPEND', json.dumps(append)).replace('REPLAY', json.dumps(replay)) + result = subprocess.run( + [NODE or 'node', '-e', script, json.dumps({ + 'restored_scene': restored_scene, 'count': count, + 'skip_unkeyed': skip_unkeyed, 'has_rows': has_rows, + })], capture_output=True, text=True, check=True, + ) + return json.loads(result.stdout) + + +@pytest.mark.parametrize('count', [0, 1, 470]) +def test_restored_complete_scene_is_not_redrawn_per_saved_tool(count): + result = replay_probe(True, count) + assert result['redraws'] == 0, result + + +def test_non_anchor_restore_still_replays_keyed_tools(): + result = replay_probe(False, 3) + assert result['appended'] == ['tool-0', 'tool-1', 'tool-2'] + + +@pytest.mark.parametrize('skip_unkeyed,has_rows', [(False, True), (True, False)]) +def test_failed_scene_or_empty_html_keeps_unkeyed_fallback(skip_unkeyed, has_rows): + result = replay_probe(False, 3, skip_unkeyed, has_rows) + assert result['appended'] == ['tool-0', 'tool-1', 'tool-2', 'unkeyed'] diff --git a/tests/test_static_asset_resolver.py b/tests/test_static_asset_resolver.py index 455a6dc7..c2e51c17 100644 --- a/tests/test_static_asset_resolver.py +++ b/tests/test_static_asset_resolver.py @@ -1,5 +1,6 @@ from __future__ import annotations +import importlib import json from pathlib import Path from types import SimpleNamespace @@ -7,7 +8,7 @@ from urllib.parse import quote import api.config as api_config import api.routes as routes -from api.updates import WEBUI_VERSION + ROOT = Path(__file__).resolve().parent.parent @@ -81,8 +82,12 @@ def test_service_worker_and_favicon_follow_selected_static_root(tmp_path, monkey monkeypatch.setattr(api_config, "get_static_root", lambda: static_root) sw_handler = _get("/sw.js") + # The route resolves api.updates dynamically on each request. Other update + # tests deliberately evict and re-import that module, so a collection-time + # alias can become stale even though the checkout and runtime module agree. + webui_version = importlib.import_module("api.updates").WEBUI_VERSION expected = sw_path.read_text(encoding="utf-8").replace( - "__WEBUI_VERSION__", quote(WEBUI_VERSION, safe="") + "__WEBUI_VERSION__", quote(webui_version, safe="") ).encode("utf-8") assert sw_handler.status == 200 assert sw_handler.header("Service-Worker-Allowed") == "/" From 23ddcf6cb1b75e1d0a8c92954cbc7b63a714c054 Mon Sep 17 00:00:00 2001 From: Brent Atchison Date: Sat, 5 Sep 2026 10:33:45 -0500 Subject: [PATCH 05/57] ci: enforce reconnect browser regression coverage --- .github/workflows/conversation-lifecycle.yml | 3 ++- TESTING.md | 14 ++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/.github/workflows/conversation-lifecycle.yml b/.github/workflows/conversation-lifecycle.yml index a1898cdd..6f49c3c8 100644 --- a/.github/workflows/conversation-lifecycle.yml +++ b/.github/workflows/conversation-lifecycle.yml @@ -34,7 +34,8 @@ jobs: live-to-final: runs-on: ubuntu-latest timeout-minutes: 10 - continue-on-error: true + # Keep existing proof rows informational; reconnect regressions fail CI. + continue-on-error: ${{ matrix.name != 'reconnect-scene-redraw' }} strategy: fail-fast: false matrix: diff --git a/TESTING.md b/TESTING.md index d8fd0ff5..61d77776 100644 --- a/TESTING.md +++ b/TESTING.md @@ -136,12 +136,14 @@ HISTORICAL_HYDRATION_TEST_BITE=break-tool-link \ python tests/browser_historical_transcript_hydration.py ``` -The dedicated `Conversation lifecycle (informational)` workflow runs the current -proof rows (`normal`, `terminal-error`, `historical-transcript-hydration`, and the -Chromium `reconnect-scene-redraw` matrix) and stays non-blocking while the public -matrix expands to additional behavior rows. The maintainer's private QA harness -remains broader; later public slices will add cancellation, compression, and -recovery coverage. +The dedicated `Conversation lifecycle (informational)` workflow keeps the existing +proof rows (`normal`, `terminal-error`, and `historical-transcript-hydration`) +non-blocking while the public matrix expands. The Chromium +`reconnect-scene-redraw` row does **not** allow failures: it exercises the real +`loadSession` reconnect path and fails the workflow on a regression. Required +merge checks remain a maintainer-controlled repository setting. +The maintainer's private QA harness remains broader; later public slices will +add cancellation, compression, and recovery coverage. ### Active-session reconnect redraw gate From cd516704199e2f152c5c3ed98c264df1a1519e62 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Wed, 26 Aug 2026 10:53:17 -0300 Subject: [PATCH 06/57] fix(profiles): guard agent.skill_utils import in skills-stats fallback (#7305) --- api/profiles.py | 29 +++++++++- tests/test_issue7305_profiles_fallback.py | 64 +++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tests/test_issue7305_profiles_fallback.py diff --git a/api/profiles.py b/api/profiles.py index f5e8432a..ad95993b 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -1862,7 +1862,34 @@ def _compute_profile_skills_stats(profile_dir: Path) -> tuple[int, int]: except Exception: pass - from agent.skill_utils import iter_skill_index_files, parse_frontmatter, skill_matches_platform + try: + from agent.skill_utils import iter_skill_index_files, parse_frontmatter, skill_matches_platform + except ImportError: + # agent source not mounted (two-container Docker, + # HERMES_WEBUI_CHAT_BACKEND=gateway): the fallback profile path must + # still work — never 500 GET /api/profiles (#7305). Best-effort local + # scan: no platform filtering, minimal frontmatter parse. + def iter_skill_index_files(root, filename="SKILL.md"): + for dirpath, _dirnames, filenames in os.walk(root, followlinks=True): + if filename in filenames: + yield Path(dirpath) / filename + + def parse_frontmatter(content): + data = {} + try: + if content.startswith("---"): + _head, _sep, rest = content.partition("\n---") + fm, _, _body = rest.partition("\n---") + for line in fm.splitlines(): + if ":" in line: + k, v = line.split(":", 1) + data[k.strip()] = v.strip().strip('"\'') + except Exception: + pass + return data, None + + def skill_matches_platform(_frontmatter): + return True seen_names = set() enabled_count = 0 diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py new file mode 100644 index 00000000..c89be6e1 --- /dev/null +++ b/tests/test_issue7305_profiles_fallback.py @@ -0,0 +1,64 @@ +"""#7305: GET /api/profiles must never 500 when agent.skill_utils is not +importable (two-container Docker without the hermes-agent source mounted).""" + +from __future__ import annotations + +import builtins +import importlib +from pathlib import Path + +import pytest + +import api.profiles as profiles + + +def _profile_dir(tmp_path, skill_name="my-skill"): + d = tmp_path / "home" + skills = d / "skills" + (skills / skill_name).mkdir(parents=True) + (skills / skill_name / "SKILL.md").write_text( + "---\nname: " + skill_name + "\ndescription: test skill\n---\nbody\n", + encoding="utf-8", + ) + return d + + +def test_skills_stats_works_without_agent_skill_utils(tmp_path): + """The fallback local scan must serve the count when the agent module is + absent — no ImportError escapes into GET /api/profiles.""" + d = _profile_dir(tmp_path) + real_import = builtins.__import__ + + def no_agent(name, *args, **kwargs): + if name == "agent.skill_utils" or name.startswith("agent."): + raise ImportError("agent not mounted (simulated Docker)") + return real_import(name, *args, **kwargs) + + importlib.reload(profiles) # ensure the module-level agent imports are re-evaluated + try: + # Patch the import used by _get_profile_skills_stats' compute path. + import api.profiles as p2 + orig = p2.__dict__.get("iter_skill_index_files") + # The fallback is installed inside the function; simulate the missing + # module by patching builtins for the module's import machinery. + with pytest.MonkeyPatch.context() as mp: + mp.setattr(builtins, "__import__", no_agent) + enabled, compatible = profiles._get_profile_skills_stats(d) + finally: + pass + assert compatible >= 1, "must count at least the local SKILL.md without agent.skill_utils" + assert enabled >= 1 + + +def test_default_profile_dict_never_raises_without_agent(tmp_path): + """_default_profile_dict() (the fallback row for GET /api/profiles) must + return a dict even with no agent module available.""" + d = _profile_dir(tmp_path) + orig = profiles._DEFAULT_HERMES_HOME + try: + profiles._DEFAULT_HERMES_HOME = d + row = profiles._default_profile_dict() + finally: + profiles._DEFAULT_HERMES_HOME = orig + assert row["name"] == "default" + assert row["skill_count"] >= 1 From d2e470b218bcad28f050e7f3d8392580758887d3 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Wed, 26 Aug 2026 10:59:01 -0300 Subject: [PATCH 07/57] test(profiles): fix ruff F401/F841 in #7305 regression --- tests/test_issue7305_profiles_fallback.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py index c89be6e1..c79d403f 100644 --- a/tests/test_issue7305_profiles_fallback.py +++ b/tests/test_issue7305_profiles_fallback.py @@ -5,7 +5,6 @@ from __future__ import annotations import builtins import importlib -from pathlib import Path import pytest @@ -37,10 +36,6 @@ def test_skills_stats_works_without_agent_skill_utils(tmp_path): importlib.reload(profiles) # ensure the module-level agent imports are re-evaluated try: # Patch the import used by _get_profile_skills_stats' compute path. - import api.profiles as p2 - orig = p2.__dict__.get("iter_skill_index_files") - # The fallback is installed inside the function; simulate the missing - # module by patching builtins for the module's import machinery. with pytest.MonkeyPatch.context() as mp: mp.setattr(builtins, "__import__", no_agent) enabled, compatible = profiles._get_profile_skills_stats(d) From 86c2252dc80c3f77dffb86ba3874290d0d6b66f4 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 18:08:19 -0300 Subject: [PATCH 08/57] fix(profiles): report skill stats as unknown when agent source is unmounted (#7305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the #7305 regression fix. - Drop the local shadow of ``agent.skill_utils`` (an ``os.walk`` + frontmatter parser + always-True platform filter). A local re-implementation cannot preserve the index walk's directory exclusions, frontmatter-name identity or platform filtering, so it reported inaccurate counts: a SKILL.md declared for another platform was counted as enabled, and support / dependency / VCS / inactive org-mirror files were counted as skills. - The Agent-absent branch now reports the stats as unknown — a stable ``(0, 0)``. GET /api/profiles still cannot 500, and the picker never shows a count it cannot verify (the UI omits the skills line when the total is 0). - Tests now pin that contract: force ``ImportError`` for ``agent.skill_utils`` (and ``hermes_cli``) and assert the unknown-stats result, the fallback row from ``_default_profile_dict()``, and the isolated-mode ``/api/profiles`` path end to end. The fixture reproduces the shapes a partial local scan gets wrong — a directory name differing from the declared ``name``, two directories declaring the same name, a declared name disabled for the webui platform, a platform-incompatible skill, and the paths the index walk prunes (support dirs of a skill package, dependency/VCS/metadata dirs, an inactive org mirror) — and a positive control runs the same tree through the real Agent-backed path so the fixture cannot degrade into a tree with nothing to count. --- api/profiles.py | 32 +-- tests/test_issue7305_profiles_fallback.py | 232 ++++++++++++++++++---- 2 files changed, 201 insertions(+), 63 deletions(-) diff --git a/api/profiles.py b/api/profiles.py index ad95993b..221a2ebc 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -1866,30 +1866,14 @@ def _compute_profile_skills_stats(profile_dir: Path) -> tuple[int, int]: from agent.skill_utils import iter_skill_index_files, parse_frontmatter, skill_matches_platform except ImportError: # agent source not mounted (two-container Docker, - # HERMES_WEBUI_CHAT_BACKEND=gateway): the fallback profile path must - # still work — never 500 GET /api/profiles (#7305). Best-effort local - # scan: no platform filtering, minimal frontmatter parse. - def iter_skill_index_files(root, filename="SKILL.md"): - for dirpath, _dirnames, filenames in os.walk(root, followlinks=True): - if filename in filenames: - yield Path(dirpath) / filename - - def parse_frontmatter(content): - data = {} - try: - if content.startswith("---"): - _head, _sep, rest = content.partition("\n---") - fm, _, _body = rest.partition("\n---") - for line in fm.splitlines(): - if ":" in line: - k, v = line.split(":", 1) - data[k.strip()] = v.strip().strip('"\'') - except Exception: - pass - return data, None - - def skill_matches_platform(_frontmatter): - return True + # HERMES_WEBUI_CHAT_BACKEND=gateway): this must never 500 GET + # /api/profiles (#7305). Report the skill stats as unknown — a stable + # (0, 0) — instead of keeping a partial shadow of agent.skill_utils + # here: a local re-implementation cannot preserve the index walk's + # exclusions, frontmatter-name identity or platform filtering, so any + # count it produced would be inaccurate. The UI omits the skills line + # when the total is 0, so the profile picker stays fully usable. + return (0, 0) seen_names = set() enabled_count = 0 diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py index c79d403f..8407de62 100644 --- a/tests/test_issue7305_profiles_fallback.py +++ b/tests/test_issue7305_profiles_fallback.py @@ -1,59 +1,213 @@ -"""#7305: GET /api/profiles must never 500 when agent.skill_utils is not -importable (two-container Docker without the hermes-agent source mounted).""" +"""#7305: GET /api/profiles must never 500 when the hermes-agent source is not +importable (two-container Docker / HERMES_WEBUI_CHAT_BACKEND=gateway). + +Contract pinned here: when ``agent.skill_utils`` is missing, the WebUI reports +the skill stats as *unknown* — a stable ``(0, 0)`` — instead of counting skills +with a local partial re-implementation of the index walk. The profile picker +still renders, and it never claims a count it cannot verify (the UI omits the +skills line when the total is 0). + +The fixture tree deliberately reproduces the shapes that a partial local scan +gets wrong: a SKILL.md whose declared ``name`` differs from its directory, two +directories declaring the same name, a declared name disabled in the profile +config, a platform-incompatible skill, and the paths the real index walk prunes +(support dirs of a skill package, dependency/VCS/metadata dirs, an inactive +org-mirror). ``test_agent_backed_path_counts_these_shapes_accurately`` is the +positive control: it runs the same tree through the real agent-backed path, so +the fixture cannot silently degrade into "a tree that never had a count to +lose". +""" from __future__ import annotations import builtins -import importlib +import pathlib import pytest import api.profiles as profiles +# How a fixture entry relates to the reported counts. +COUNTED = "counted" # yields a logical skill; contributes to compatible/enabled +WALK_PRUNED = "walk-pruned" # never yielded by agent.skill_utils' index walk +PLATFORM_FILTERED = "platform-filtered" # walked, but incompatible with this host -def _profile_dir(tmp_path, skill_name="my-skill"): - d = tmp_path / "home" - skills = d / "skills" - (skills / skill_name).mkdir(parents=True) - (skills / skill_name / "SKILL.md").write_text( - "---\nname: " + skill_name + "\ndescription: test skill\n---\nbody\n", - encoding="utf-8", - ) - return d +# path -> (declared frontmatter name, platform restriction or None, outcome) +SKILLS_TREE = { + # Declared name differs from the directory name; that declared name is + # disabled for the webui platform in config.yaml below. + "alpha-dir/SKILL.md": ("my-skill", None, COUNTED), + # A second directory declaring the same name — one logical skill. + "beta-dir/SKILL.md": ("my-skill", None, COUNTED), + # Platform-incompatible: must not be offered (or counted) on this host. + "gamma-dir/SKILL.md": ("gamma-skill", "some-other-platform", PLATFORM_FILTERED), + # A plain, enabled skill. + "delta-dir/SKILL.md": ("delta-skill", None, COUNTED), + # Support dir of a skill package: disclosure-only, not a standalone skill. + "delta-dir/references/notes/SKILL.md": ("support-note", None, WALK_PRUNED), + # Dependency / virtualenv / VCS / metadata dirs are excluded wholesale, and + # an org mirror without its .active_org marker is token-gated shut. + "node_modules/dep/SKILL.md": ("vendored-skill", None, WALK_PRUNED), + ".venv/lib/pkg/SKILL.md": ("venv-skill", None, WALK_PRUNED), + ".git/hooks/SKILL.md": ("hook-skill", None, WALK_PRUNED), + "_org/other-org/org-skill/SKILL.md": ("org-skill", None, WALK_PRUNED), +} + +# The one declared name the profile config disables for the webui platform. +DISABLED_NAMES = {"my-skill"} + +CONFIG_YAML = ( + "profile:\n" + " name: default\n" + "skills:\n" + " platform_disabled:\n" + " webui:\n" + " - my-skill\n" +) + +CONFIG_WITHOUT_DISABLE = "profile:\n name: default\n" -def test_skills_stats_works_without_agent_skill_utils(tmp_path): - """The fallback local scan must serve the count when the agent module is - absent — no ImportError escapes into GET /api/profiles.""" - d = _profile_dir(tmp_path) +def _expected_walked() -> set[str]: + return { + rel + for rel, (_, _, outcome) in SKILLS_TREE.items() + if outcome != WALK_PRUNED + } + + +def _expected_names(outcome: str) -> set[str]: + return {name for name, _, entry_outcome in SKILLS_TREE.values() if entry_outcome == outcome} + + +@pytest.fixture() +def agent_absent(monkeypatch): + """Simulate the agent-less deployment: importing anything under ``agent.`` + raises ``ImportError``, even when the source tree is present locally.""" real_import = builtins.__import__ - def no_agent(name, *args, **kwargs): - if name == "agent.skill_utils" or name.startswith("agent."): - raise ImportError("agent not mounted (simulated Docker)") + def _no_agent(name, *args, **kwargs): + if name == "agent" or str(name).startswith("agent."): + raise ImportError(f"{name} not mounted (simulated two-container Docker)") return real_import(name, *args, **kwargs) - importlib.reload(profiles) # ensure the module-level agent imports are re-evaluated - try: - # Patch the import used by _get_profile_skills_stats' compute path. - with pytest.MonkeyPatch.context() as mp: - mp.setattr(builtins, "__import__", no_agent) - enabled, compatible = profiles._get_profile_skills_stats(d) - finally: - pass - assert compatible >= 1, "must count at least the local SKILL.md without agent.skill_utils" - assert enabled >= 1 + monkeypatch.setattr(builtins, "__import__", _no_agent) -def test_default_profile_dict_never_raises_without_agent(tmp_path): +@pytest.fixture() +def hermes_cli_absent(monkeypatch): + """Simulate the same deployment's other half: no ``hermes_cli`` either.""" + real_import = builtins.__import__ + + def _no_cli(name, *args, **kwargs): + if name == "hermes_cli" or str(name).startswith("hermes_cli."): + raise ImportError(f"{name} not mounted (simulated two-container Docker)") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _no_cli) + + +def _write_profile(tmp_path: pathlib.Path, dir_name: str = "home") -> pathlib.Path: + """Materialise SKILLS_TREE + config.yaml as a profile home.""" + home = tmp_path / dir_name + skills = home / "skills" + for rel, (name, platform, _outcome) in SKILLS_TREE.items(): + skill_md = skills / rel + skill_md.parent.mkdir(parents=True, exist_ok=True) + lines = ["---", f"name: {name}", "description: issue 7305 fixture"] + if platform: + lines += ["platforms:", f" - {platform}"] + lines += ["---", "body", ""] + skill_md.write_text("\n".join(lines), encoding="utf-8") + (home / "config.yaml").write_text(CONFIG_YAML, encoding="utf-8") + return home + + +def test_skills_stats_are_unknown_without_agent_skill_utils(tmp_path, agent_absent): + """Without agent.skill_utils the stats are (0, 0) — never a guessed count, + no matter which fixtures a partial local scan would have counted.""" + home = _write_profile(tmp_path) + + assert profiles._get_profile_skills_stats(home) == (0, 0) + + +def test_default_profile_dict_never_raises_without_agent(tmp_path, agent_absent, monkeypatch): """_default_profile_dict() (the fallback row for GET /api/profiles) must - return a dict even with no agent module available.""" - d = _profile_dir(tmp_path) - orig = profiles._DEFAULT_HERMES_HOME - try: - profiles._DEFAULT_HERMES_HOME = d - row = profiles._default_profile_dict() - finally: - profiles._DEFAULT_HERMES_HOME = orig + return a usable row with unknown skill stats instead of raising.""" + home = _write_profile(tmp_path) + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", home) + + row = profiles._default_profile_dict() + assert row["name"] == "default" - assert row["skill_count"] >= 1 + assert row["path"] == str(home) + assert row["is_default"] is True + assert row["skill_count"] == 0 + assert row["enabled_skills"] == 0 + assert row["total_skills"] == 0 + + +def test_list_profiles_api_isolated_mode_without_agent_or_cli( + tmp_path, agent_absent, hermes_cli_absent, monkeypatch +): + """The #7305 path end to end: isolated profile mode with neither + agent.skill_utils nor hermes_cli importable must still return the + default-only row instead of propagating ImportError out of the route.""" + home = _write_profile(tmp_path, dir_name="default") + monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: True) + monkeypatch.setattr(profiles, "_INITIAL_HERMES_HOME", str(home)) + + rows = profiles.list_profiles_api() + + assert [r["name"] for r in rows] == ["default"] + assert rows[0]["is_active"] is True + assert rows[0]["skill_count"] == 0 + assert rows[0]["enabled_skills"] == 0 + assert rows[0]["total_skills"] == 0 + + +def test_agent_backed_path_counts_these_shapes_accurately(tmp_path): + """Positive control (agent source present): the fixture above is a real + semantic trap, not a tree with nothing to count. + + - the index walk yields exactly the non-pruned SKILL.md files, so the + support/dependency/VCS/org-mirror files a naive local ``os.walk`` would + pick up are invisible to the count; + - the platform-incompatible and duplicate-declared-name files do not add to + the compatible total, which therefore stays below the walk's file count; + - identity comes from frontmatter ``name``: the disabled ``my-skill`` (a + declared name, not a directory) is compatible but not enabled; + - dropping that config disable flips it to enabled without changing the + compatible total. + """ + pytest.importorskip("agent.skill_utils") + from agent.skill_utils import iter_skill_index_files + + home = _write_profile(tmp_path) + skills = home / "skills" + + walked = { + p.relative_to(skills).as_posix() for p in iter_skill_index_files(skills, "SKILL.md") + } + on_disk = {p.relative_to(skills).as_posix() for p in skills.rglob("SKILL.md")} + + expected_walked = _expected_walked() + expected_compatible = _expected_names(COUNTED) + expected_enabled = expected_compatible - DISABLED_NAMES + + # The fixture must contain paths a naive scan would over-count... + assert on_disk > walked, "fixture must include SKILL.md files the index walk prunes" + # ...and the walk must still see exactly the paths it is documented to keep. + assert walked == expected_walked + + enabled, compatible = profiles._compute_profile_skills_stats(home) + + assert compatible == len(expected_compatible) + assert compatible < len(walked) + assert enabled == len(expected_enabled) + + (home / "config.yaml").write_text(CONFIG_WITHOUT_DISABLE, encoding="utf-8") + enabled_all, compatible_all = profiles._compute_profile_skills_stats(home) + + assert compatible_all == compatible + assert enabled_all == len(expected_compatible) From aeab2fb99f7bbf484f174663681c28421d9e1364 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 18:27:46 -0300 Subject: [PATCH 09/57] test(profiles): derive skill-count expectations from the agent walk output (#7305) The positive control asserted on_disk > walked, i.e. it froze the agent package's index-walk exclusion list. In an environment whose agent.skill_utils yields every SKILL.md (no pruning) both sets are equal and the assertion failed, which is what broke CI on the previous head. Expectations are now derived from the paths the walk actually yielded, so the test keeps asserting the property that belongs to this code - the compatible total stays strictly below the walked file count via platform filtering and frontmatter-name deduplication - without freezing another package's exclusion policy. --- tests/test_issue7305_profiles_fallback.py | 49 +++++++++++++++-------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/tests/test_issue7305_profiles_fallback.py b/tests/test_issue7305_profiles_fallback.py index 8407de62..3a3331e3 100644 --- a/tests/test_issue7305_profiles_fallback.py +++ b/tests/test_issue7305_profiles_fallback.py @@ -76,10 +76,6 @@ def _expected_walked() -> set[str]: } -def _expected_names(outcome: str) -> set[str]: - return {name for name, _, entry_outcome in SKILLS_TREE.values() if entry_outcome == outcome} - - @pytest.fixture() def agent_absent(monkeypatch): """Simulate the agent-less deployment: importing anything under ``agent.`` @@ -170,11 +166,16 @@ def test_agent_backed_path_counts_these_shapes_accurately(tmp_path): """Positive control (agent source present): the fixture above is a real semantic trap, not a tree with nothing to count. - - the index walk yields exactly the non-pruned SKILL.md files, so the - support/dependency/VCS/org-mirror files a naive local ``os.walk`` would - pick up are invisible to the count; + The fixture's expectations are derived from the paths the agent's index + walk actually yields, because that walk (and its exclusion list) belongs to + the agent package and differs between deployments. What this asserts: + + - the compatible total stays strictly below the walked file count — the + support/dependency/VCS/org-mirror files a naive local ``os.walk`` picks + up are either pruned by the walk or, when the walk keeps them, still not + enough to reach the walk's file count; - the platform-incompatible and duplicate-declared-name files do not add to - the compatible total, which therefore stays below the walk's file count; + the compatible total; - identity comes from frontmatter ``name``: the disabled ``my-skill`` (a declared name, not a directory) is compatible but not enabled; - dropping that config disable flips it to enabled without changing the @@ -191,19 +192,35 @@ def test_agent_backed_path_counts_these_shapes_accurately(tmp_path): } on_disk = {p.relative_to(skills).as_posix() for p in skills.rglob("SKILL.md")} - expected_walked = _expected_walked() - expected_compatible = _expected_names(COUNTED) + # The index walk itself belongs to the agent package and its exclusion list + # is version-dependent, so this test does not freeze it: every expectation + # below is derived from the paths the walk actually yielded. (Removing the + # local mirror of that walk is the point of #7305 — the fallback reported + # (0, 0) instead of guessing.) + assert _expected_walked() <= walked, "the walk must keep the non-pruned skills" + assert walked <= on_disk + + expected_compatible = { + name + for rel, (name, platform, _outcome) in SKILLS_TREE.items() + if rel in walked and platform is None + } expected_enabled = expected_compatible - DISABLED_NAMES - # The fixture must contain paths a naive scan would over-count... - assert on_disk > walked, "fixture must include SKILL.md files the index walk prunes" - # ...and the walk must still see exactly the paths it is documented to keep. - assert walked == expected_walked + # The fixture is a real semantic trap: the compatible total must stay below + # the walked file count. If the walk pruned the support/dependency/VCS/org + # files the count drops because those files are gone; if it did not, the + # count still drops because the platform-incompatible file is filtered out + # and the two directories declaring `my-skill` collapse to one logical + # skill. + assert len(expected_compatible) < len(walked), ( + "fixture must make the compatible total fall below the walked file count" + ) enabled, compatible = profiles._compute_profile_skills_stats(home) - assert compatible == len(expected_compatible) - assert compatible < len(walked) + assert compatible == len(expected_compatible), (compatible, expected_compatible) + assert compatible < len(on_disk) assert enabled == len(expected_enabled) (home / "config.yaml").write_text(CONFIG_WITHOUT_DISABLE, encoding="utf-8") From ef9341b2ab1e59ae84cd3437a24cca00c7bdefbd Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Sun, 6 Sep 2026 14:12:12 +0000 Subject: [PATCH 10/57] fix(sqlite): keep state.db readers strictly read-only and stop idle polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Readers never upgrade to writers. open_state_db_readonly() opens file:...?mode=ro only; a read-only open failure propagates instead of silently retrying with a write-capable handle on the multi-GiB WAL database the agent is streaming into. The session listing no longer self-heals idx_messages_session with CREATE INDEX (a writer lock held for minutes on a large messages table) — a missing index degrades to the pre-aggregated path. Cron sidebar info, insights and deep health checks in api/routes.py use the same read-only opener. Watcher (api/gateway_watcher.py): _poll_once returns without touching the database when nobody is subscribed, and forces a fresh full projection on the next subscription. Maintenance (scripts/ensure_state_db_read_indexes.py): the covering read indexes (idx_messages_session, idx_messages_session_role with NOCASE, idx_sessions_webui_fingerprint) are created only under --confirm-drained, optionally holding an exclusive flock on --lock-file for deployments that serialise agent turns on a lock file; existing indexes are verified for shape/collation and coverage (EXPLAIN QUERY PLAN), never silently replaced. Tests: tests/test_state_db_readonly_readers.py (read-only failure never retries writable; no poll without subscribers; listing creates no index; index tool requires drain, is idempotent, works without a lock file and refuses a held lock; cron sidebar read is mode=ro). Neighbouring tests that asserted the retired writable fallback / self-heal writer (test_issue3887_index_prime, test_issue5455_listing_readonly_connection, test_issue5455_lineage_readonly_reads, test_issue2628_cli_sessions_perf, test_issue1494_state_db_fd_leak) now assert the read-only contract; test_issue3506 subscribes before polling. --- api/agent_sessions.py | 57 +------- api/gateway_watcher.py | 6 + api/routes.py | 7 +- scripts/ensure_state_db_read_indexes.py | 98 ++++++++++++++ tests/test_issue1494_state_db_fd_leak.py | 6 +- tests/test_issue2628_cli_sessions_perf.py | 3 +- tests/test_issue3506_memory_and_watcher.py | 1 + tests/test_issue3887_index_prime.py | 34 +++-- .../test_issue5455_lineage_readonly_reads.py | 27 ++-- ...t_issue5455_listing_readonly_connection.py | 45 ++++--- tests/test_state_db_readonly_readers.py | 124 ++++++++++++++++++ 11 files changed, 303 insertions(+), 105 deletions(-) create mode 100644 scripts/ensure_state_db_read_indexes.py create mode 100644 tests/test_state_db_readonly_readers.py diff --git a/api/agent_sessions.py b/api/agent_sessions.py index 97086795..310df64d 100644 --- a/api/agent_sessions.py +++ b/api/agent_sessions.py @@ -24,30 +24,17 @@ def open_state_db_readonly(db_path: Path, log: logging.Logger | None = None) -> Same rationale as the session-listing path (#5455): a write-capable handle on the multi-GB, WAL ``state.db`` while the agent streams into it adds needless checkpoint/lock surface. The read-only ``file:...?mode=ro`` URI - avoids that. Falls back to a writable connection (and warns) if the - read-only open fails, so callers never lose data on exotic filesystems. + avoids that. Read failures propagate; a reader never upgrades to a writer. The caller must ensure ``db_path`` exists — this raises ``FileNotFoundError`` - for a missing path rather than letting the writable fallback below create an - empty, writable ``state.db`` there (a ghost DB in the agent's HOME). The - fallback is only for an *existing* DB whose read-only open fails on an exotic - filesystem, so a real read never loses data. + for a missing path rather than creating a ghost database. Callers own the returned connection (wrap it in ``contextlib.closing``). """ - log = log or logger if not db_path.exists(): raise FileNotFoundError(f"agent state.db not found: {db_path}") read_only_uri = f"{db_path.resolve().as_uri()}?mode=ro" - try: - return sqlite3.connect(read_only_uri, uri=True) - except sqlite3.Error as exc: - log.warning( - "agent state.db read-only open failed for %s; falling back to writable connection: %s", - db_path, - exc, - ) - return sqlite3.connect(str(db_path)) + return sqlite3.connect(read_only_uri, uri=True) MESSAGING_SOURCES = { @@ -549,21 +536,7 @@ def read_importable_agent_session_rows( return [] log = log or logger - # Open read-only for this projection/listing path: it is a pure read, and - # holding a write-capable handle on the live (multi-GB, WAL) state.db while - # the agent streams into it adds needless checkpoint/lock surface (#5455). - # The defensive index self-heal below still runs, but through a separate - # short-lived writable connection on the rare missing-index path only. - read_only_uri = f"{db_path.resolve().as_uri()}?mode=ro" - try: - conn = sqlite3.connect(read_only_uri, uri=True) - except sqlite3.Error as exc: - log.warning( - "agent session listing read-only open failed for %s; falling back to writable connection: %s", - db_path, - exc, - ) - conn = sqlite3.connect(str(db_path)) + conn = open_state_db_readonly(db_path, log=log) with closing(conn): conn.row_factory = sqlite3.Row cur = conn.cursor() @@ -613,11 +586,7 @@ def read_importable_agent_session_rows( use_messages_join = messages_has_session_id count_col = 'id' if 'id' in message_cols else 'session_id' - # Defensive index prime (#3887). The normal candidate-ordering shape uses - # the agent's standard ``idx_messages_session ON messages(session_id, - # timestamp)`` index; without it, large cron-only scans degrade badly. - # Writable dbs self-heal by recreating the index. Read-only or locked dbs - # fall back to the pre-aggregated cron-only path below instead of failing. + # Index creation belongs to explicit drained maintenance, never a read. messages_index_present = False if messages_has_session_id and messages_has_timestamp: try: @@ -625,21 +594,7 @@ def read_importable_agent_session_rows( messages_index_present = any(str(row[1]) == "idx_messages_session" for row in cur.fetchall()) except sqlite3.Error: messages_index_present = False - if not messages_index_present: - # Self-heal via a separate writable connection so the common - # (index-present) path keeps its read-only handle. On a truly - # read-only/locked db this fails and we degrade to the - # pre-aggregated cron-only path below, exactly as before. - try: - with closing(sqlite3.connect(str(db_path))) as _heal: - _heal.execute( - "CREATE INDEX IF NOT EXISTS idx_messages_session " - "ON messages(session_id, timestamp)" - ) - _heal.commit() - messages_index_present = True - except sqlite3.Error: - pass # read-only db / locked / older schema — degrade gracefully + if use_messages_join: actual_count_expr = f"COUNT(m.{count_col})" diff --git a/api/gateway_watcher.py b/api/gateway_watcher.py index b52f5b0c..0354c337 100644 --- a/api/gateway_watcher.py +++ b/api/gateway_watcher.py @@ -317,6 +317,12 @@ class GatewayWatcher: protects projection fields (notably role-derived CLI visibility) that the agent's existing index cannot see. """ + with self._sub_lock: + has_subscribers = bool(self._subscribers) + if not has_subscribers: + # Force fresh projection on the next subscription, without touching DB. + self._last_full_projection_at = None + return False db_path = self._state_db_path # A watcher may start before the agent has created state.db. Publishing an # empty first snapshot would make an already-rendered sidebar disappear; diff --git a/api/routes.py b/api/routes.py index 7d30ab90..0804cb2e 100644 --- a/api/routes.py +++ b/api/routes.py @@ -46,6 +46,7 @@ from api.agent_sessions import ( _looks_like_default_cli_title, is_cli_session_row, is_cli_session_row_visible, + open_state_db_readonly, read_session_lineage_report, ) from api.compression_anchor import visible_messages_for_anchor @@ -320,7 +321,7 @@ def _latest_cron_session_info_for_jobs( if not db_path or not Path(db_path).exists(): return {jid: {"session_id": "", "message_count": None} for jid in requested} try: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute("PRAGMA table_info(sessions)") @@ -11897,7 +11898,7 @@ def _handle_insights(handler, parsed) -> bool: from api.models import _active_state_db_path db_path = _active_state_db_path() if db_path and db_path.exists(): - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() # cache_read_tokens may not exist on older agent state DBs; @@ -12532,7 +12533,7 @@ def _deep_health_checks(stream_check: dict | None = None) -> tuple[dict, bool]: "ms": round((time.time() - t0) * 1000, 1), } else: - with closing(sqlite3.connect(str(db_path))) as conn: + with closing(open_state_db_readonly(db_path)) as conn: conn.execute("PRAGMA schema_version").fetchone() checks["state_db"] = { "status": "ok", diff --git a/scripts/ensure_state_db_read_indexes.py b/scripts/ensure_state_db_read_indexes.py new file mode 100644 index 00000000..87aad2cd --- /dev/null +++ b/scripts/ensure_state_db_read_indexes.py @@ -0,0 +1,98 @@ +#!/usr/bin/env python3 +"""Create the covering read indexes on the agent ``state.db`` in an explicit, +drained maintenance window. + +The WebUI read paths (session listing, lineage reads, gateway watcher, cron +sidebar, insights, health) never create indexes themselves: ``CREATE INDEX`` +on a multi-GiB ``messages`` table holds the SQLite writer lock for minutes, +which stalls the agent streaming into the same WAL database. Index +maintenance is therefore an operator action run while the agent is idle. + +Usage:: + + python scripts/ensure_state_db_read_indexes.py --db ~/.hermes/state.db \\ + --confirm-drained [--lock-file /path/to/agent-activity.lock] + +``--confirm-drained`` is mandatory. When ``--lock-file`` is given, the tool +takes an exclusive non-blocking ``flock`` on that path so that a deployment +which serialises agent turns on a lock file cannot start a turn mid-rebuild. + +Existing indexes are verified for table, key shape and collation and must be +covering (``EXPLAIN QUERY PLAN``); an incompatible index is reported, never +silently replaced. +""" +import argparse +from contextlib import closing, nullcontext +import fcntl +import json +from pathlib import Path +import sqlite3 + +INDEXES = { + "idx_messages_session": ("messages", (("session_id", "BINARY"), ("timestamp", "BINARY"))), + "idx_messages_session_role": ("messages", (("session_id", "BINARY"), ("role", "NOCASE"))), + "idx_sessions_webui_fingerprint": ("sessions", tuple((c, "BINARY") for c in + ("source", "id", "message_count", "last_activity_at"))), +} + + +def _exclusive_lock(lock_file): + if lock_file is None: + return nullcontext() + handle = open(lock_file, "a+") + try: + fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BaseException: + handle.close() + raise + return closing(handle) + + +def ensure_read_indexes(db_path, *, confirmed_drained=False, lock_file=None): + if not confirmed_drained: + raise RuntimeError("requires --confirm-drained") + db_path = Path(db_path).resolve(strict=True) + statuses = {} + with _exclusive_lock(lock_file): + with closing(sqlite3.connect(db_path.as_uri() + "?mode=rw", uri=True)) as db: + db.execute("BEGIN IMMEDIATE") + try: + for name, (table, keys) in INDEXES.items(): + existing = db.execute("SELECT tbl_name FROM sqlite_master WHERE name=?", (name,)).fetchone() + if existing: + actual = tuple((r[2], r[4]) for r in db.execute(f"PRAGMA index_xinfo({name})") if r[5]) + flags = next((r for r in db.execute(f"PRAGMA index_list({table})") if r[1] == name), None) + if existing[0] != table or actual != keys or not flags or flags[2] or flags[4]: + raise RuntimeError(f"Incompatible index: {name}") + statuses[name] = "existing" + else: + columns = ", ".join(f"{col} COLLATE {collation}" for col, collation in keys) + db.execute(f"CREATE INDEX {name} ON {table}({columns})") + statuses[name] = "created" + columns = ", ".join(col for col, _ in keys) + plan = db.execute(f"EXPLAIN QUERY PLAN SELECT {columns} FROM {table} INDEXED BY {name}").fetchall() + if not any(f"COVERING INDEX {name}" in row[3] for row in plan): + raise RuntimeError(f"Index is not covering: {name}") + db.commit() + except BaseException: + db.rollback() + raise + return statuses + + +def main(): + parser = argparse.ArgumentParser(description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("--db", type=Path, required=True, help="path to the agent state.db") + parser.add_argument("--lock-file", type=Path, default=None, + help="optional lock file to hold exclusively (flock) while indexes are built") + parser.add_argument("--confirm-drained", action="store_true", + help="assert that no agent turn is running against this database") + args = parser.parse_args() + result = ensure_read_indexes(args.db, confirmed_drained=args.confirm_drained, + lock_file=args.lock_file) + print(json.dumps(result, sort_keys=True)) + + +if __name__ == "__main__": + main() diff --git a/tests/test_issue1494_state_db_fd_leak.py b/tests/test_issue1494_state_db_fd_leak.py index 1691e43d..6ba92759 100644 --- a/tests/test_issue1494_state_db_fd_leak.py +++ b/tests/test_issue1494_state_db_fd_leak.py @@ -163,9 +163,9 @@ def test_read_importable_agent_session_rows_closes_connection(tmp_path, tracking read_importable_agent_session_rows(db) _assert_all_closed(tracking_sqlite, "read_importable_agent_session_rows") - # #5455: missing-index self-heal uses one separate short-lived RW connection - # in addition to the five read-only listing connections. - assert len(tracking_sqlite.instances) == 6 + # #5455: the listing is strictly read-only, including when indexes are + # missing — five calls, five read-only connections, no self-heal writer. + assert len(tracking_sqlite.instances) == 5 def test_read_session_lineage_metadata_closes_connection(tmp_path, tracking_sqlite): diff --git a/tests/test_issue2628_cli_sessions_perf.py b/tests/test_issue2628_cli_sessions_perf.py index 70f4513f..90dbaf2c 100644 --- a/tests/test_issue2628_cli_sessions_perf.py +++ b/tests/test_issue2628_cli_sessions_perf.py @@ -212,7 +212,8 @@ def test_importable_agent_rows_push_sidebar_limit_into_sql(tmp_path): assert 'included == ("cron",)' in src assert "not messages_index_present" in src assert "PRAGMA index_list(messages)" in src - assert "CREATE INDEX IF NOT EXISTS idx_messages_session" in src + # Listing is a pure read: index creation lives in the drained maintenance tool. + assert "CREATE INDEX IF NOT EXISTS idx_messages_session" not in src assert "_CRON_PREAGGREGATE_CANDIDATE_ORDER_MIN_MESSAGES" not in src assert "MAX(mx.timestamp) FROM messages mx WHERE mx.session_id = s.id" in src assert "candidate_limit = max(result_limit * 8, result_limit)" in src diff --git a/tests/test_issue3506_memory_and_watcher.py b/tests/test_issue3506_memory_and_watcher.py index 5fa60884..46eb0040 100644 --- a/tests/test_issue3506_memory_and_watcher.py +++ b/tests/test_issue3506_memory_and_watcher.py @@ -534,6 +534,7 @@ def test_poll_loop_skips_projection_when_unchanged(tmp_path, monkeypatch): monkeypatch.setattr(gw, "_get_agent_sessions_from_db", fake_projection) w = gw.GatewayWatcher(state_db_path=db) + w.subscribe() # Idle watchers deliberately do not poll without observers. assert w._poll_once(now=1.0) is True assert projected == [True] diff --git a/tests/test_issue3887_index_prime.py b/tests/test_issue3887_index_prime.py index 399f3406..d3641989 100644 --- a/tests/test_issue3887_index_prime.py +++ b/tests/test_issue3887_index_prime.py @@ -74,28 +74,38 @@ def _messages_indexes(path): return {r[0] for r in rows} -def test_prime_creates_missing_index(tmp_path): - """A db missing idx_messages_session gets it primed on the first scan.""" +def test_listing_does_not_create_missing_index(tmp_path, monkeypatch): + """A missing index must never turn a sidebar read into a schema write. + + CREATE INDEX on a multi-GiB ``messages`` table holds the SQLite writer lock + for minutes; index maintenance belongs to the explicit drained tool + (``scripts/ensure_state_db_read_indexes.py``), not to a listing. + """ db = tmp_path / "state.db" _full_schema_db(db) assert "idx_messages_session" not in _messages_indexes(db) + connect_calls = [] + real_connect = agent_sessions.sqlite3.connect + + def recording_connect(*args, **kwargs): + connect_calls.append((args, kwargs)) + return real_connect(*args, **kwargs) + + monkeypatch.setattr(agent_sessions.sqlite3, "connect", recording_connect) rows = agent_sessions.read_importable_agent_session_rows( db, limit=20, exclude_sources=None ) + # Snapshot before the verifier below opens its own (non-listing) handle. + listing_connect_calls = list(connect_calls) # Listing still returns the sessions ... assert {r["id"] for r in rows} == {"sess0", "sess1", "sess2"} - # ... and the index now exists on (session_id, timestamp). - assert "idx_messages_session" in _messages_indexes(db) - conn = sqlite3.connect(str(db)) - try: - sql = conn.execute( - "SELECT sql FROM sqlite_master WHERE name='idx_messages_session'" - ).fetchone()[0] - finally: - conn.close() - assert "session_id" in sql and "timestamp" in sql + # ... without mutating the schema or opening a write-capable connection. + assert "idx_messages_session" not in _messages_indexes(db) + assert len(listing_connect_calls) == 1 + assert "mode=ro" in str(listing_connect_calls[0][0][0]) + assert listing_connect_calls[0][1].get("uri") is True def test_prime_is_noop_when_index_exists(tmp_path): diff --git a/tests/test_issue5455_lineage_readonly_reads.py b/tests/test_issue5455_lineage_readonly_reads.py index 35d3af71..be2ec777 100644 --- a/tests/test_issue5455_lineage_readonly_reads.py +++ b/tests/test_issue5455_lineage_readonly_reads.py @@ -9,7 +9,6 @@ lineage-metadata reads, and the gateway-watcher fingerprint projection (a 5s poll), were still opening a read-WRITE connection. This shared them onto the same ``open_state_db_readonly`` helper. """ -import logging import sqlite3 from contextlib import closing @@ -98,25 +97,25 @@ def test_helper_encodes_special_path_chars(tmp_path, monkeypatch): assert calls[0]["target"].endswith("?mode=ro") -def test_helper_falls_back_to_writable_and_logs(tmp_path, monkeypatch, caplog): +def test_helper_never_falls_back_to_writable(tmp_path, monkeypatch): + """A read-only open failure propagates; the helper never retries with a + write-capable handle on the live WAL database.""" db = tmp_path / "state.db" _make_lineage_db(db) - real_connect = sqlite3.connect + calls = [] def fail_read_only(target, *args, **kwargs): - if kwargs.get("uri"): - raise sqlite3.OperationalError("synthetic read-only URI failure") - return real_connect(target, *args, **kwargs) + calls.append({"target": str(target), "uri": bool(kwargs.get("uri"))}) + raise sqlite3.OperationalError("synthetic read-only URI failure") monkeypatch.setattr(agent_sessions.sqlite3, "connect", fail_read_only) - with caplog.at_level(logging.WARNING, logger="api.agent_sessions"): - conn = open_state_db_readonly(db) - try: - assert conn.execute("SELECT COUNT(*) FROM sessions").fetchone()[0] == 2 - finally: - conn.close() - assert "read-only open failed" in caplog.text - assert "synthetic read-only URI failure" in caplog.text + with pytest.raises(sqlite3.OperationalError, match="synthetic read-only URI failure"): + open_state_db_readonly(db) + + # Exactly one attempt, and it was the read-only URI form. + assert len(calls) == 1 + assert calls[0]["uri"] is True + assert calls[0]["target"].endswith("?mode=ro") def test_helper_raises_on_missing_db_instead_of_creating_a_ghost(tmp_path): diff --git a/tests/test_issue5455_listing_readonly_connection.py b/tests/test_issue5455_listing_readonly_connection.py index fc2cb263..06bdaa9e 100644 --- a/tests/test_issue5455_listing_readonly_connection.py +++ b/tests/test_issue5455_listing_readonly_connection.py @@ -6,13 +6,12 @@ re-run a defensive ``CREATE INDEX`` self-heal on every sidebar build. Holding a write-capable handle while the agent streams into the same DB adds needless checkpoint/lock surface. -The listing path now opens the DB read-only (``file:...?mode=ro``) and only -self-heals a missing ``idx_messages_session`` through a SEPARATE short-lived -writable connection. With the index present (the normal case) the read path -performs zero writes; when the index is missing the self-heal still runs and -rows still come back. +The listing path opens the DB read-only (``file:...?mode=ro``) and never +upgrades to a writable handle: a read-only open failure propagates, and a +missing ``idx_messages_session`` degrades to the pre-aggregated path instead +of being self-healed. Index maintenance is an explicit drained operation +(``scripts/ensure_state_db_read_indexes.py``). """ -import logging import sqlite3 import api.agent_sessions as agent_sessions @@ -92,24 +91,27 @@ def test_listing_read_only_uri_encodes_special_path_chars(tmp_path, monkeypatch) assert calls[0]["target"].endswith("?mode=ro") -def test_read_only_open_fallback_is_logged(tmp_path, monkeypatch, caplog): +def test_read_only_open_failure_never_retries_with_writable_connection(tmp_path, monkeypatch): db = tmp_path / "state.db" _make_db(db, with_index=True) - real_connect = sqlite3.connect + calls = [] def fail_read_only(target, *args, **kwargs): - if kwargs.get("uri"): - raise sqlite3.OperationalError("synthetic read-only URI failure") - return real_connect(target, *args, **kwargs) + calls.append({"target": str(target), "uri": bool(kwargs.get("uri"))}) + raise sqlite3.OperationalError("synthetic read-only URI failure") monkeypatch.setattr(agent_sessions.sqlite3, "connect", fail_read_only) - with caplog.at_level(logging.WARNING, logger="api.agent_sessions"): - out = read_importable_agent_session_rows(db, exclude_sources=None) + try: + read_importable_agent_session_rows(db, exclude_sources=None) + raise AssertionError("read-only open failure unexpectedly recovered") + except sqlite3.OperationalError as exc: + assert "synthetic read-only URI failure" in str(exc) - assert "cli-1" in {r["id"] for r in out} - assert "read-only open failed" in caplog.text - assert "synthetic read-only URI failure" in caplog.text + # Exactly one attempt, and it was the read-only URI form. + assert len(calls) == 1 + assert calls[0]["uri"] is True + assert "mode=ro" in calls[0]["target"] def test_index_present_performs_no_writable_connection(tmp_path, monkeypatch): @@ -124,7 +126,7 @@ def test_index_present_performs_no_writable_connection(tmp_path, monkeypatch): assert all(c["uri"] and "mode=ro" in c["target"] for c in calls), calls -def test_missing_index_self_heals_via_separate_connection(tmp_path, monkeypatch): +def test_missing_index_degrades_without_writable_connection(tmp_path, monkeypatch): db = tmp_path / "state.db" _make_db(db, with_index=False) calls = _record_connects(monkeypatch) @@ -133,12 +135,13 @@ def test_missing_index_self_heals_via_separate_connection(tmp_path, monkeypatch) # Rows still come back... assert "cli-1" in {r["id"] for r in out} - # ...and the self-heal ran through a separate writable (non-ro) connection. - assert any((not c["uri"]) and "mode=ro" not in c["target"] for c in calls), calls - # The index now exists on disk. + # ...without any implicit schema-maintenance writer. + assert calls + assert all(c["uri"] and "mode=ro" in c["target"] for c in calls), calls + # A missing index stays a maintenance concern, not a listing side effect. verify = sqlite3.connect(str(db)) try: names = {row[1] for row in verify.execute("PRAGMA index_list(messages)")} finally: verify.close() - assert "idx_messages_session" in names + assert "idx_messages_session" not in names diff --git a/tests/test_state_db_readonly_readers.py b/tests/test_state_db_readonly_readers.py new file mode 100644 index 00000000..25dd8d20 --- /dev/null +++ b/tests/test_state_db_readonly_readers.py @@ -0,0 +1,124 @@ +"""state.db readers stay strictly read-only and idle watchers do not poll. + +Contract under test: + +* ``open_state_db_readonly`` opens ``file:...?mode=ro`` once and propagates a + failure — it never retries with a write-capable handle; +* the gateway watcher does not touch the database while nobody is subscribed; +* the session listing never runs ``CREATE INDEX`` (a writer lock held for + minutes on a large ``messages`` table) — a missing index degrades instead; +* index maintenance is an explicit drained operation + (``scripts/ensure_state_db_read_indexes.py``); +* the cron sidebar read in ``api/routes.py`` uses the read-only opener. +""" +import fcntl +import sqlite3 +from contextlib import closing + +import pytest + + +def test_readonly_open_failure_never_retries_writable(tmp_path, monkeypatch): + from api import agent_sessions + path = tmp_path / "state.db" + path.touch() + calls = [] + + def connect(*args, **kwargs): + calls.append((args, kwargs)) + raise sqlite3.OperationalError("readonly unavailable") + + monkeypatch.setattr(agent_sessions.sqlite3, "connect", connect) + with pytest.raises(sqlite3.OperationalError): + agent_sessions.open_state_db_readonly(path) + assert len(calls) == 1 + assert calls[0][1] == {"uri": True} + + +def test_no_subscribers_means_no_database_poll(tmp_path, monkeypatch): + from api import gateway_watcher as watcher + path = tmp_path / "state.db" + path.touch() + instance = watcher.GatewayWatcher(hermes_home=tmp_path) + monkeypatch.setattr(watcher, "_cheap_change_fingerprint", lambda *a: pytest.fail("polled DB")) + monkeypatch.setattr(watcher, "_get_agent_sessions_from_db", lambda *a: pytest.fail("projected payload")) + assert instance._poll_once(now=500) is False + + +def test_listing_does_not_create_indexes(tmp_path): + from api.agent_sessions import read_importable_agent_session_rows + path = tmp_path / "state.db" + with closing(sqlite3.connect(path)) as db: + db.executescript(""" + CREATE TABLE sessions(id TEXT PRIMARY KEY, title TEXT, model TEXT, + message_count INTEGER, started_at REAL, source TEXT); + CREATE TABLE messages(id INTEGER PRIMARY KEY, session_id TEXT, timestamp REAL, role TEXT); + INSERT INTO sessions VALUES ('one', 'Test', 'test', 2, 1, 'cli'); + INSERT INTO messages VALUES (1, 'one', 2, 'user'); + INSERT INTO messages VALUES (2, 'one', 3, 'assistant'); + """) + rows = read_importable_agent_session_rows(path) + assert rows[0]["id"] == "one" + with closing(sqlite3.connect(path)) as db: + assert db.execute("PRAGMA index_list(messages)").fetchall() == [] + + +def _maintenance_schema(path): + with closing(sqlite3.connect(path)) as db: + db.executescript(""" + CREATE TABLE sessions(source TEXT, id TEXT, message_count INTEGER, last_activity_at REAL); + CREATE TABLE messages(session_id TEXT, timestamp REAL, role TEXT); + """) + + +def test_index_maintenance_requires_drain_and_is_idempotent(tmp_path): + from scripts.ensure_state_db_read_indexes import ensure_read_indexes + path = tmp_path / "state.db" + _maintenance_schema(path) + with pytest.raises(RuntimeError, match="confirm-drained"): + ensure_read_indexes(path) + lock = tmp_path / "turns.lock" + first = ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + second = ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + assert set(first.values()) == {"created"} + assert set(second.values()) == {"existing"} + assert "idx_messages_session_role" in first + + +def test_index_maintenance_without_lock_file_still_requires_drain(tmp_path): + from scripts.ensure_state_db_read_indexes import ensure_read_indexes + path = tmp_path / "state.db" + _maintenance_schema(path) + result = ensure_read_indexes(path, confirmed_drained=True) + assert set(result.values()) == {"created"} + + +def test_index_maintenance_refuses_when_lock_is_held(tmp_path): + from scripts.ensure_state_db_read_indexes import ensure_read_indexes + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + with open(lock, "a+") as holder: + fcntl.flock(holder, fcntl.LOCK_EX | fcntl.LOCK_NB) + with pytest.raises(BlockingIOError): + ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + with closing(sqlite3.connect(path)) as db: + assert db.execute("PRAGMA index_list(messages)").fetchall() == [] + + +def test_cron_sidebar_read_uses_readonly_connection(tmp_path, monkeypatch): + from api import routes + path = tmp_path / 'state.db' + with closing(sqlite3.connect(path)) as db: + db.execute('CREATE TABLE sessions(id TEXT, source TEXT)') + monkeypatch.setattr(routes, '_active_state_db_path', lambda: path) + connect = sqlite3.connect + calls = [] + def recording(*args, **kwargs): + calls.append((args, kwargs)) + return connect(*args, **kwargs) + monkeypatch.setattr(sqlite3, 'connect', recording) + routes._latest_cron_session_info_for_jobs(['test']) + assert len(calls) == 1 + assert 'mode=ro' in calls[0][0][0] + assert calls[0][1].get('uri') is True From f5a7ef2049808e8621266eb2997b8cc44688d2fd Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 16 Sep 2026 22:44:55 +0000 Subject: [PATCH 11/57] fix(sqlite): pre-aggregate every missing-index listing, UNC-safe read-only URI, portable index tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #7445 (nesquena-hermes CHANGES_REQUESTED + Greptile). api/agent_sessions.py - The read-only pre-aggregated `latest_messages` candidate ordering now applies to every missing-index projection (default sidebar, gateway watcher, cron/webhook/kanban views), not only `include_sources == ("cron",)`. Since the listing no longer self-heals the index, the correlated `MAX(mx.timestamp)` scan re-ran on every load: 9.86 s on a 5,000-session / 20,000-message DB vs 0.044 s after this change, with identical selected rows and order. The indexed seek is unchanged. - `state_db_file_uri()` / `state_db_readonly_uri()` replace `Path.as_uri()`: a UNC `HERMES_HOME` produced `file://server/share/...` and CPython's bundled SQLite (3.11-3.13 Windows builds, no SQLITE_ALLOW_URI_AUTHORITY) rejected the authority, so every agent-backed session vanished. The builder emits the empty-authority `file:////server/share/state.db?mode=ro`, keeps `file:///C:/...` for drive letters, strips `\\?\` / `\\?\UNC\` prefixes, quotes `?`/`#`, and leaves POSIX output byte-identical. Strict `mode=ro`, no writable fallback. scripts/ensure_state_db_read_indexes.py - `fcntl` / `msvcrt` are imported conditionally; the module imports and the no-`--lock-file` path runs on native Windows. `--lock-file` uses `flock` on POSIX, `msvcrt.locking(LK_NBLCK)` on Windows, and fails with a clear RuntimeError when neither exists. The rw open reuses the UNC-aware URI builder. docs/troubleshooting.md - Operator section: read-only contract, degraded missing-index listing, drained maintenance prerequisites, lock behaviour, failure modes, UNC. Tests: tests/test_pr7445_readonly_review_followups.py (19 tests, red before this change: correlated-subquery plan, missing URI builder, ImportError on fcntl). Query-plan contract via EXPLAIN QUERY PLAN — no CORRELATED SCALAR SUBQUERY on missing-index listings, identical rows to the indexed listing; platform-parametrised URI cases; maintenance module imported with `sys.modules["fcntl"] = None`. test_issue2628 no longer pins the cron-only gate; the flock-held test skips without fcntl. --- api/agent_sessions.py | 59 +++- docs/troubleshooting.md | 30 ++ scripts/ensure_state_db_read_indexes.py | 35 ++- tests/test_issue2628_cli_sessions_perf.py | 3 +- .../test_pr7445_readonly_review_followups.py | 262 ++++++++++++++++++ tests/test_state_db_readonly_readers.py | 2 +- 6 files changed, 379 insertions(+), 12 deletions(-) create mode 100644 tests/test_pr7445_readonly_review_followups.py diff --git a/api/agent_sessions.py b/api/agent_sessions.py index 310df64d..6c5691d9 100644 --- a/api/agent_sessions.py +++ b/api/agent_sessions.py @@ -1,8 +1,10 @@ """Shared helpers for reading Hermes Agent sessions from state.db.""" import logging import sqlite3 +import sys from contextlib import closing -from pathlib import Path +from pathlib import Path, PurePosixPath, PureWindowsPath +from urllib.parse import quote logger = logging.getLogger(__name__) @@ -18,6 +20,49 @@ logger = logging.getLogger(__name__) _SOURCE_COLUMN_WARNED_DB_PATHS: set[str] = set() +def state_db_file_uri(db_path, platform: str | None = None) -> str: + """Build the ``file:`` URI (no query string) for an absolute ``state.db`` path. + + ``Path.as_uri()`` is not usable here: for a UNC share it emits + ``file://server/share/state.db`` and SQLite rejects any non-local URI + authority unless compiled with ``SQLITE_ALLOW_URI_AUTHORITY`` (the + CPython 3.11-3.13 Windows builds are not). SQLite instead accepts the + *empty*-authority spelling ``file:////server/share/state.db``, whose path + ``//server/share/...`` the Windows VFS opens as the UNC name. Drive-letter + paths keep the documented ``file:///C:/...`` form and POSIX paths are + unchanged. Only ``/`` survives unescaped, so ``?`` and ``#`` in path + components cannot leak into the query string. + + ``platform`` defaults to the running interpreter; tests pass it explicitly + so the Windows shapes are checked from any host. + """ + platform = platform or sys.platform + if platform == "win32": + win = PureWindowsPath(str(db_path)) + drive = win.drive + if drive.startswith("\\\\?\\"): + # Extended-length prefix: "\\?\C:" or "\\?\UNC\server\share". + drive = drive[4:] + if drive.upper().startswith("UNC\\"): + drive = "\\\\" + drive[4:] + parts = win.parts[1:] # drop the anchor, keep the path components + if drive.startswith("\\\\"): + host_share = drive[2:].replace("\\", "/") + posix_path = "//" + "/".join((host_share, *parts)) + else: + posix_path = "/" + "/".join((drive, *parts)) + # ``:`` stays literal so the drive letter keeps SQLite's documented + # ``file:///C:/...`` shape (``Path.as_uri()`` leaves it unescaped too). + return "file://" + quote(posix_path, safe="/:") + posix_path = PurePosixPath(str(db_path)).as_posix() + return "file://" + quote(posix_path, safe="/") + + +def state_db_readonly_uri(db_path, platform: str | None = None) -> str: + """Strict read-only (``mode=ro``) form of :func:`state_db_file_uri`.""" + return state_db_file_uri(db_path, platform=platform) + "?mode=ro" + + def open_state_db_readonly(db_path: Path, log: logging.Logger | None = None) -> sqlite3.Connection: """Open the live agent ``state.db`` read-only for a pure-read projection. @@ -33,8 +78,7 @@ def open_state_db_readonly(db_path: Path, log: logging.Logger | None = None) -> """ if not db_path.exists(): raise FileNotFoundError(f"agent state.db not found: {db_path}") - read_only_uri = f"{db_path.resolve().as_uri()}?mode=ro" - return sqlite3.connect(read_only_uri, uri=True) + return sqlite3.connect(state_db_readonly_uri(db_path.resolve()), uri=True) MESSAGING_SOURCES = { @@ -620,7 +664,6 @@ def read_importable_agent_session_rows( where_clauses = ["s.source IS NOT NULL"] params: list[object] = [] - included = () if include_sources: included = tuple(str(source) for source in include_sources if source) if included: @@ -634,10 +677,16 @@ def read_importable_agent_session_rows( where_clauses.append(f"s.source NOT IN ({placeholders})") params.extend(excluded) + # Without ``idx_messages_session`` the correlated ``MAX(mx.timestamp)`` + # candidate ordering rescans ``messages`` once per session (seconds on + # a few thousand sessions). Every missing-index projection — default + # sidebar, gateway watcher, cron/webhook/kanban views — orders through + # one read-only pre-aggregation pass instead; the listing never creates + # the index itself (that is drained maintenance, see + # ``scripts/ensure_state_db_read_indexes.py``). use_preaggregated_candidate_order = ( use_messages_join and messages_has_timestamp - and included == ("cron",) and not messages_index_present ) if use_preaggregated_candidate_order: diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f3335f98..c6fcffd8 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -211,6 +211,36 @@ For a foreground `python3 bootstrap.py`, stop it with Ctrl-C and start it again. --- +## Agent sessions list slowly (or the `state.db` read index is missing) + +**Symptom.** The sidebar's imported/CLI session list takes seconds per refresh on a large Hermes profile, or a log line says a `state.db` read failed. Sessions still appear; nothing is lost. + +**Why.** Every WebUI reader of the agent's `state.db` (session listing, transcript reads, lineage, gateway watcher, cron sidebar, insights, health) opens it strictly read-only (`file:...?mode=ro`). A reader never upgrades to a write-capable handle and never creates an index: on a multi-GiB `messages` table `CREATE INDEX` holds the SQLite writer lock for minutes and stalls the agent streaming into the same WAL database. When the agent's standard `idx_messages_session` index is missing (older agent, hand-rebuilt or re-imported DB), listings degrade to a bounded one-pass pre-aggregation — slower than the indexed seek, but read-only. A read-only open failure propagates to the caller's existing error boundary (the listing returns empty for that profile) instead of silently reopening the file writable. + +**Diagnostic.** + +```bash +sqlite3 "file:$HOME/.hermes/state.db?mode=ro" "PRAGMA index_list(messages)" +``` + +`idx_messages_session` should be listed. If it is not, the agent has not created it and WebUI will not create it for you. + +**Fix.** Create the covering read indexes in an explicit drained maintenance window: stop the agent (and any gateway/cron runner) writing to that `state.db`, then run: + +```bash +python3 scripts/ensure_state_db_read_indexes.py --db ~/.hermes/state.db --confirm-drained +``` + +- `--confirm-drained` is mandatory: it is your assertion that no agent turn is running against the database. The tool does not verify it. +- `--lock-file PATH` (optional) additionally holds an exclusive non-blocking lock on `PATH` (`flock` on POSIX, `msvcrt.locking` on Windows) for deployments that serialise agent turns on a lock file; a held lock makes the tool exit without touching the database. Without `--lock-file` no lock primitive is required, so the script runs on native Windows as well. +- The tool opens the database `mode=rw` (never `rwc`): a mistyped path raises instead of creating an empty database. Indexes are created inside one `BEGIN IMMEDIATE` transaction and rolled back on any error. +- It is idempotent and prints a JSON status per index (`created` / `existing`). An existing index with a different table, key shape or collation is reported as `Incompatible index` and never replaced; an index that is not covering (`EXPLAIN QUERY PLAN`) is reported as `Index is not covering`. +- Windows UNC profiles (`HERMES_HOME=\\server\share\...`) are supported: readers and this tool build the empty-authority URI `file:////server/share/state.db` that the bundled SQLite accepts. + +**When to file a bug.** File a WebUI bug if the listing stays slow after the tool reports `existing` for `idx_messages_session`, if the tool reports `Incompatible index` on an untouched agent-created database, or if a read-only open fails on a local path. Include the tool's JSON output, the `PRAGMA index_list(messages)` result, and the sanitized error text. + +--- + ## 404 after login when password auth is enabled **Symptom.** After enabling password authentication (`HERMES_WEBUI_PASSWORD`), logging in redirects to `/sessions` and the browser shows a `404 not found` error instead of the chat interface. diff --git a/scripts/ensure_state_db_read_indexes.py b/scripts/ensure_state_db_read_indexes.py index 87aad2cd..2513b0fc 100644 --- a/scripts/ensure_state_db_read_indexes.py +++ b/scripts/ensure_state_db_read_indexes.py @@ -14,8 +14,10 @@ Usage:: --confirm-drained [--lock-file /path/to/agent-activity.lock] ``--confirm-drained`` is mandatory. When ``--lock-file`` is given, the tool -takes an exclusive non-blocking ``flock`` on that path so that a deployment -which serialises agent turns on a lock file cannot start a turn mid-rebuild. +takes an exclusive non-blocking lock on that path (``flock`` on POSIX, +``msvcrt.locking`` on Windows) so that a deployment which serialises agent +turns on a lock file cannot start a turn mid-rebuild. Without ``--lock-file`` +no lock primitive is needed, so the script runs on every supported platform. Existing indexes are verified for table, key shape and collation and must be covering (``EXPLAIN QUERY PLAN``); an incompatible index is reported, never @@ -23,10 +25,24 @@ silently replaced. """ import argparse from contextlib import closing, nullcontext -import fcntl import json from pathlib import Path import sqlite3 +import sys + +try: # POSIX + import fcntl +except ImportError: # native Windows + fcntl = None +try: # Windows + import msvcrt +except ImportError: # POSIX + msvcrt = None + +_REPO_ROOT = Path(__file__).resolve().parents[1] +if str(_REPO_ROOT) not in sys.path: + sys.path.insert(0, str(_REPO_ROOT)) +from api.agent_sessions import state_db_file_uri # same UNC-aware URI as the readers INDEXES = { "idx_messages_session": ("messages", (("session_id", "BINARY"), ("timestamp", "BINARY"))), @@ -39,9 +55,17 @@ INDEXES = { def _exclusive_lock(lock_file): if lock_file is None: return nullcontext() + if fcntl is None and msvcrt is None: + raise RuntimeError("--lock-file needs fcntl (POSIX) or msvcrt (Windows); " + "neither is available on this interpreter") handle = open(lock_file, "a+") try: - fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + if fcntl is not None: + fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + else: + # Lock one byte at offset 0 without blocking; raises OSError when held. + handle.seek(0) + msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) except BaseException: handle.close() raise @@ -54,7 +78,8 @@ def ensure_read_indexes(db_path, *, confirmed_drained=False, lock_file=None): db_path = Path(db_path).resolve(strict=True) statuses = {} with _exclusive_lock(lock_file): - with closing(sqlite3.connect(db_path.as_uri() + "?mode=rw", uri=True)) as db: + # mode=rw (not rwc): never create a ghost database at a mistyped path. + with closing(sqlite3.connect(state_db_file_uri(db_path) + "?mode=rw", uri=True)) as db: db.execute("BEGIN IMMEDIATE") try: for name, (table, keys) in INDEXES.items(): diff --git a/tests/test_issue2628_cli_sessions_perf.py b/tests/test_issue2628_cli_sessions_perf.py index 90dbaf2c..556e5509 100644 --- a/tests/test_issue2628_cli_sessions_perf.py +++ b/tests/test_issue2628_cli_sessions_perf.py @@ -209,7 +209,8 @@ def test_importable_agent_rows_push_sidebar_limit_into_sql(tmp_path): assert "JOIN candidates c ON c.id = s.id" in src assert "latest_messages AS" in src assert "LEFT JOIN latest_messages lm ON lm.session_id = s.id" in src - assert 'included == ("cron",)' in src + # Pre-aggregation covers every missing-index projection, not only cron (#7445 review). + assert 'included == ("cron",)' not in src assert "not messages_index_present" in src assert "PRAGMA index_list(messages)" in src # Listing is a pure read: index creation lives in the drained maintenance tool. diff --git a/tests/test_pr7445_readonly_review_followups.py b/tests/test_pr7445_readonly_review_followups.py new file mode 100644 index 00000000..a2ecd444 --- /dev/null +++ b/tests/test_pr7445_readonly_review_followups.py @@ -0,0 +1,262 @@ +"""Review follow-ups for the strictly read-only ``state.db`` readers (PR #7445). + +Contract under test: + +* every missing-index projection (default sidebar, webhook, kanban, …), not + only the cron view, orders its candidate window through the read-only + pre-aggregated ``latest_messages`` CTE — never through the correlated + ``MAX(mx.timestamp)`` scalar subquery that rescans ``messages`` per session; + the selected rows and their order stay identical to the indexed listing; +* ``state_db_readonly_uri`` builds a ``file:`` URI SQLite accepts on every + supported platform: POSIX unchanged, Windows drive letters as + ``file:///C:/...``, UNC shares with an *empty* authority + (``file:////server/share/...``) because CPython's bundled SQLite rejects any + non-local authority, extended-length ``\\\\?\\`` prefixes stripped; the + query string is always the strict ``mode=ro``; +* ``scripts/ensure_state_db_read_indexes.py`` imports and runs its no-lock path + on a platform without ``fcntl`` (native Windows), and refuses ``--lock-file`` + there with a clear error instead of an ``ImportError`` at import time. +""" +import importlib +import sqlite3 +import sys +from contextlib import closing + +import pytest + +import api.agent_sessions as agent_sessions + + +# -------------------------------------------------------------------------- +# 1. Missing-index projections use the pre-aggregated candidate ordering. +# -------------------------------------------------------------------------- + +def _make_state_db(path, *, sessions, source, with_index): + with closing(sqlite3.connect(str(path))) as conn: + conn.executescript( + """ + CREATE TABLE sessions( + id TEXT PRIMARY KEY, title TEXT, model TEXT, message_count INTEGER, + started_at REAL, source TEXT, session_source TEXT, + parent_session_id TEXT, ended_at REAL, end_reason TEXT); + CREATE TABLE messages( + id INTEGER PRIMARY KEY, session_id TEXT, role TEXT, + content TEXT, timestamp REAL); + """ + ) + for i in range(sessions): + sid = f"{source}-{i:04d}" + conn.execute( + "INSERT INTO sessions VALUES (?,?,?,?,?,?,?,NULL,NULL,NULL)", + (sid, f"Session {i}", "m", 3, 1000.0 + i, source, source), + ) + for j in range(3): + conn.execute( + "INSERT INTO messages(session_id, role, content, timestamp) VALUES (?,?,?,?)", + (sid, "user" if j == 0 else "assistant", "x", 1000.0 + i + j / 10), + ) + # A resumed old session: started first, but its last message is newest. + conn.execute( + "INSERT INTO sessions VALUES (?,?,?,?,?,?,?,NULL,NULL,NULL)", + (f"{source}-resumed", "Resumed", "m", 2, 1.0, source, source), + ) + conn.execute( + "INSERT INTO messages(session_id, role, content, timestamp) VALUES (?,?,?,?)", + (f"{source}-resumed", "user", "old", 1.0), + ) + conn.execute( + "INSERT INTO messages(session_id, role, content, timestamp) VALUES (?,?,?,?)", + (f"{source}-resumed", "assistant", "recent", 5000.0), + ) + if with_index: + conn.execute("CREATE INDEX idx_messages_session ON messages(session_id, timestamp)") + conn.commit() + + +def _capture_listing_sql(monkeypatch): + """Record the SQL statements the listing runs (trace callback, read-only).""" + statements = [] + real_connect = sqlite3.connect + + def tracing_connect(*args, **kwargs): + conn = real_connect(*args, **kwargs) + conn.set_trace_callback(statements.append) + return conn + + monkeypatch.setattr(agent_sessions.sqlite3, "connect", tracing_connect) + return statements + + +def _candidate_query_plan(db_path, statements): + listing_sql = [s for s in statements if "FROM sessions s" in s] + assert len(listing_sql) == 1, listing_sql + with closing(sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)) as conn: + return [row[3] for row in conn.execute("EXPLAIN QUERY PLAN " + listing_sql[0])] + + +@pytest.mark.parametrize( + ("source", "kwargs"), + [ + ("cli", {}), # default /api/sessions projection + ("webhook", {"exclude_sources": None, "include_sources": ("webhook",)}), + ("kanban", {"exclude_sources": None, "include_sources": ("kanban",)}), + ("cli", {"exclude_sources": None}), # gateway watcher / diagnostic view + ], +) +def test_missing_index_projection_uses_preaggregated_ordering(tmp_path, monkeypatch, source, kwargs): + indexed = tmp_path / "indexed.db" + unindexed = tmp_path / "unindexed.db" + _make_state_db(indexed, sessions=60, source=source, with_index=True) + _make_state_db(unindexed, sessions=60, source=source, with_index=False) + + expected = agent_sessions.read_importable_agent_session_rows(indexed, limit=20, **kwargs) + assert expected[0]["id"] == f"{source}-resumed" + assert len(expected) == 20 + + statements = _capture_listing_sql(monkeypatch) + actual = agent_sessions.read_importable_agent_session_rows(unindexed, limit=20, **kwargs) + + # Same rows, same order as the indexed listing. + assert [r["id"] for r in actual] == [r["id"] for r in expected] + assert [r["actual_message_count"] for r in actual] == [r["actual_message_count"] for r in expected] + assert [r["last_activity"] for r in actual] == [r["last_activity"] for r in expected] + + plan = _candidate_query_plan(unindexed, statements) + # One pre-aggregation pass over messages, never a per-candidate rescan. + assert not any("CORRELATED SCALAR SUBQUERY" in step for step in plan), plan + assert any("latest_messages" in step for step in plan), plan + # The listing stayed a pure read: no index appeared. + with closing(sqlite3.connect(str(unindexed))) as conn: + assert conn.execute("PRAGMA index_list(messages)").fetchall() == [] + + +def test_indexed_projection_keeps_the_correlated_index_seek(tmp_path, monkeypatch): + """With ``idx_messages_session`` present the indexed seek stays in place.""" + db = tmp_path / "state.db" + _make_state_db(db, sessions=10, source="cli", with_index=True) + statements = _capture_listing_sql(monkeypatch) + rows = agent_sessions.read_importable_agent_session_rows(db, limit=20) + assert rows[0]["id"] == "cli-resumed" + plan = _candidate_query_plan(db, statements) + assert not any("latest_messages" in step for step in plan), plan + assert any("idx_messages_session" in step for step in plan), plan + + +# -------------------------------------------------------------------------- +# 2. Read-only URI construction is platform-aware and strictly mode=ro. +# -------------------------------------------------------------------------- + +@pytest.mark.parametrize( + ("platform", "path", "expected"), + [ + ("linux", "/home/me/.hermes/state.db", "file:///home/me/.hermes/state.db?mode=ro"), + ("darwin", "/Users/me/state dir #1/state?.db", + "file:///Users/me/state%20dir%20%231/state%3F.db?mode=ro"), + ("win32", r"C:\Users\me\.hermes\state.db", "file:///C:/Users/me/.hermes/state.db?mode=ro"), + ("win32", r"C:\a b\state#?.db", "file:///C:/a%20b/state%23%3F.db?mode=ro"), + ("win32", r"\\server\share\hermes\state.db", "file:////server/share/hermes/state.db?mode=ro"), + ("win32", r"\\server\share\a b\state#.db", "file:////server/share/a%20b/state%23.db?mode=ro"), + ("win32", r"\\?\C:\x\state.db", "file:///C:/x/state.db?mode=ro"), + ("win32", r"\\?\UNC\server\share\state.db", "file:////server/share/state.db?mode=ro"), + ], +) +def test_state_db_readonly_uri_by_platform(platform, path, expected): + assert agent_sessions.state_db_readonly_uri(path, platform=platform) == expected + + +def test_state_db_readonly_uri_never_carries_a_host_authority(): + uri = agent_sessions.state_db_readonly_uri(r"\\server\share\state.db", platform="win32") + assert uri.startswith("file:////") + assert "file://server" not in uri + assert uri.endswith("?mode=ro") + + +def test_open_state_db_readonly_uses_platform_uri_builder(tmp_path, monkeypatch): + path = tmp_path / "state.db" + with closing(sqlite3.connect(str(path))) as conn: + conn.execute("CREATE TABLE t(x)") + calls = [] + real_connect = sqlite3.connect + + def recording(target, *args, **kwargs): + calls.append((str(target), kwargs)) + return real_connect(target, *args, **kwargs) + + monkeypatch.setattr(agent_sessions.sqlite3, "connect", recording) + with closing(agent_sessions.open_state_db_readonly(path)) as conn: + assert conn.execute("SELECT COUNT(*) FROM t").fetchone() == (0,) + assert calls == [(agent_sessions.state_db_readonly_uri(path.resolve()), {"uri": True})] + assert calls[0][0].endswith("?mode=ro") + + +def test_empty_authority_uri_opens_on_this_platform(tmp_path): + """SQLite accepts the empty-authority form the UNC path relies on.""" + path = tmp_path / "state.db" + with closing(sqlite3.connect(str(path))) as conn: + conn.execute("CREATE TABLE t(x)") + uri = agent_sessions.state_db_readonly_uri(path.resolve()) + with closing(sqlite3.connect(uri, uri=True)) as conn: + assert conn.execute("SELECT COUNT(*) FROM t").fetchone() == (0,) + with pytest.raises(sqlite3.OperationalError): + conn.execute("INSERT INTO t VALUES (1)") + + +# -------------------------------------------------------------------------- +# 3. The index maintenance script imports and runs without fcntl. +# -------------------------------------------------------------------------- + +def _maintenance_schema(path): + with closing(sqlite3.connect(str(path))) as conn: + conn.executescript( + """ + CREATE TABLE sessions(source TEXT, id TEXT, message_count INTEGER, last_activity_at REAL); + CREATE TABLE messages(session_id TEXT, timestamp REAL, role TEXT); + """ + ) + + +@pytest.fixture +def maintenance_without_fcntl(monkeypatch): + """Import the maintenance module as a platform without ``fcntl`` sees it.""" + name = "scripts.ensure_state_db_read_indexes" + monkeypatch.setitem(sys.modules, "fcntl", None) + monkeypatch.delitem(sys.modules, name, raising=False) # restores a prior real import + module = importlib.import_module(name) + try: + yield module + finally: + # Drop the fcntl-less copy before monkeypatch restores the original entry. + if sys.modules.get(name) is module: + del sys.modules[name] + + +def test_maintenance_imports_and_runs_without_fcntl(tmp_path, maintenance_without_fcntl): + module = maintenance_without_fcntl + path = tmp_path / "state.db" + _maintenance_schema(path) + result = module.ensure_read_indexes(path, confirmed_drained=True) + assert set(result.values()) == {"created"} + with closing(sqlite3.connect(str(path))) as conn: + names = {row[1] for row in conn.execute("PRAGMA index_list(messages)")} + assert "idx_messages_session" in names + + +def test_maintenance_refuses_lock_file_without_a_lock_primitive(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + monkeypatch.setitem(sys.modules, "msvcrt", None) + path = tmp_path / "state.db" + _maintenance_schema(path) + with pytest.raises(RuntimeError, match="--lock-file"): + module.ensure_read_indexes(path, confirmed_drained=True, lock_file=tmp_path / "turns.lock") + with closing(sqlite3.connect(str(path))) as conn: + assert conn.execute("PRAGMA index_list(messages)").fetchall() == [] + + +def test_maintenance_cli_without_lock_file_works_without_fcntl(tmp_path, maintenance_without_fcntl, monkeypatch, capsys): + module = maintenance_without_fcntl + path = tmp_path / "state.db" + _maintenance_schema(path) + monkeypatch.setattr(sys, "argv", ["ensure_state_db_read_indexes.py", "--db", str(path), "--confirm-drained"]) + module.main() + out = capsys.readouterr().out + assert '"idx_messages_session": "created"' in out diff --git a/tests/test_state_db_readonly_readers.py b/tests/test_state_db_readonly_readers.py index 25dd8d20..c7d1c7a0 100644 --- a/tests/test_state_db_readonly_readers.py +++ b/tests/test_state_db_readonly_readers.py @@ -11,7 +11,6 @@ Contract under test: (``scripts/ensure_state_db_read_indexes.py``); * the cron sidebar read in ``api/routes.py`` uses the read-only opener. """ -import fcntl import sqlite3 from contextlib import closing @@ -94,6 +93,7 @@ def test_index_maintenance_without_lock_file_still_requires_drain(tmp_path): def test_index_maintenance_refuses_when_lock_is_held(tmp_path): + fcntl = pytest.importorskip("fcntl", reason="flock-held scenario needs a POSIX interpreter") from scripts.ensure_state_db_read_indexes import ensure_read_indexes path = tmp_path / "state.db" _maintenance_schema(path) From 70146b74d9555036dde7d9e691d5bf343d342f07 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Thu, 17 Sep 2026 22:26:45 +0000 Subject: [PATCH 12/57] fix(sqlite): initialise an empty --lock-file before the Windows byte lock On the msvcrt branch of scripts/ensure_state_db_read_indexes.py the tool locked byte 0 of --lock-file right after open(..., "a+"). When that open had just created the file (or the file was empty) the lock range lay beyond EOF, which Windows refuses, so the documented locked-maintenance workflow raised instead of acquiring the lock (greptile P1). Write one byte when the lock file is empty, then seek(0) and lock as before. Only the Windows branch changes; POSIX flock is untouched and an existing (deployment-owned) lock file is never rewritten. Tests (tests/test_pr7445_readonly_review_followups.py): a strict msvcrt stand-in that refuses to lock past EOF proves the lock is taken on byte 0 of a fresh empty file, that an existing lock file keeps its content, and that a held lock still propagates before the database is touched. --- docs/troubleshooting.md | 2 +- scripts/ensure_state_db_read_indexes.py | 7 ++ .../test_pr7445_readonly_review_followups.py | 91 ++++++++++++++++++- 3 files changed, 98 insertions(+), 2 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index c6fcffd8..618efa9c 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -232,7 +232,7 @@ python3 scripts/ensure_state_db_read_indexes.py --db ~/.hermes/state.db --confir ``` - `--confirm-drained` is mandatory: it is your assertion that no agent turn is running against the database. The tool does not verify it. -- `--lock-file PATH` (optional) additionally holds an exclusive non-blocking lock on `PATH` (`flock` on POSIX, `msvcrt.locking` on Windows) for deployments that serialise agent turns on a lock file; a held lock makes the tool exit without touching the database. Without `--lock-file` no lock primitive is required, so the script runs on native Windows as well. +- `--lock-file PATH` (optional) additionally holds an exclusive non-blocking lock on `PATH` (`flock` on POSIX, `msvcrt.locking` on Windows) for deployments that serialise agent turns on a lock file; a held lock makes the tool exit without touching the database. Without `--lock-file` no lock primitive is required, so the script runs on native Windows as well. On Windows the lock covers byte 0 of `PATH`; a new or empty lock file is initialised with one byte first (an existing lock file is never rewritten). - The tool opens the database `mode=rw` (never `rwc`): a mistyped path raises instead of creating an empty database. Indexes are created inside one `BEGIN IMMEDIATE` transaction and rolled back on any error. - It is idempotent and prints a JSON status per index (`created` / `existing`). An existing index with a different table, key shape or collation is reported as `Incompatible index` and never replaced; an index that is not covering (`EXPLAIN QUERY PLAN`) is reported as `Index is not covering`. - Windows UNC profiles (`HERMES_HOME=\\server\share\...`) are supported: readers and this tool build the empty-authority URI `file:////server/share/state.db` that the bundled SQLite accepts. diff --git a/scripts/ensure_state_db_read_indexes.py b/scripts/ensure_state_db_read_indexes.py index 2513b0fc..2eedc9a4 100644 --- a/scripts/ensure_state_db_read_indexes.py +++ b/scripts/ensure_state_db_read_indexes.py @@ -26,6 +26,7 @@ silently replaced. import argparse from contextlib import closing, nullcontext import json +import os from pathlib import Path import sqlite3 import sys @@ -64,6 +65,12 @@ def _exclusive_lock(lock_file): fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) else: # Lock one byte at offset 0 without blocking; raises OSError when held. + # ``a+`` may have just created an empty file, and Windows cannot lock + # a range beyond EOF, so make sure byte 0 exists first. An existing + # (deployment-owned) lock file is left untouched. + if os.fstat(handle.fileno()).st_size == 0: + handle.write("\n") + handle.flush() handle.seek(0) msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) except BaseException: diff --git a/tests/test_pr7445_readonly_review_followups.py b/tests/test_pr7445_readonly_review_followups.py index a2ecd444..f99c1548 100644 --- a/tests/test_pr7445_readonly_review_followups.py +++ b/tests/test_pr7445_readonly_review_followups.py @@ -15,9 +15,13 @@ Contract under test: query string is always the strict ``mode=ro``; * ``scripts/ensure_state_db_read_indexes.py`` imports and runs its no-lock path on a platform without ``fcntl`` (native Windows), and refuses ``--lock-file`` - there with a clear error instead of an ``ImportError`` at import time. + there with a clear error instead of an ``ImportError`` at import time; +* on the ``msvcrt`` branch the tool locks byte 0 of the lock file only after + making sure that byte exists, so a fresh (empty) ``--lock-file`` is acquired + without relying on the CRT's beyond-EOF locking behaviour. """ import importlib +import os import sqlite3 import sys from contextlib import closing @@ -260,3 +264,88 @@ def test_maintenance_cli_without_lock_file_works_without_fcntl(tmp_path, mainten module.main() out = capsys.readouterr().out assert '"idx_messages_session": "created"' in out + + +# -------------------------------------------------------------------------- +# 4. The Windows lock branch acquires a fresh (empty) lock file. +# -------------------------------------------------------------------------- + +class _StrictMsvcrt: + """Stand-in for ``msvcrt`` that, like Windows, refuses to lock past EOF. + + Records every ``locking`` call with the file size at the current offset so + the test can prove byte 0 existed when the lock was taken. + """ + + LK_NBLCK = 2 + LK_UNLCK = 0 + + def __init__(self): + self.calls = [] + + def locking(self, fd, mode, nbytes): + offset = os.lseek(fd, 0, os.SEEK_CUR) + size = os.fstat(fd).st_size + self.calls.append((mode, offset, nbytes, size)) + if mode == self.LK_NBLCK and offset + nbytes > size: + raise OSError(36, "Resource deadlock avoided: lock region beyond end of file") + + +def test_windows_lock_branch_acquires_a_fresh_empty_lock_file(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + assert module.fcntl is None # the fixture already routes the tool to the msvcrt branch + fake = _StrictMsvcrt() + monkeypatch.setattr(module, "msvcrt", fake) + + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + assert not lock.exists() + + result = module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + + assert set(result.values()) == {"created"} + # Exactly one non-blocking lock on byte 0, taken while that byte existed. + assert fake.calls == [(fake.LK_NBLCK, 0, 1, 1)] + assert lock.stat().st_size == 1 + with closing(sqlite3.connect(str(path))) as conn: + names = {row[1] for row in conn.execute("PRAGMA index_list(messages)")} + assert "idx_messages_session" in names + + +def test_windows_lock_branch_keeps_an_existing_lock_file_intact(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + fake = _StrictMsvcrt() + monkeypatch.setattr(module, "msvcrt", fake) + + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + lock.write_bytes(b"deployment-owned\n") + + module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + + assert fake.calls == [(fake.LK_NBLCK, 0, 1, len(b"deployment-owned\n"))] + assert lock.read_bytes() == b"deployment-owned\n" + + +def test_windows_lock_branch_reports_a_held_lock_without_touching_the_db(tmp_path, maintenance_without_fcntl, monkeypatch): + module = maintenance_without_fcntl + + class _HeldMsvcrt(_StrictMsvcrt): + def locking(self, fd, mode, nbytes): + super().locking(fd, mode, nbytes) + raise PermissionError(13, "Permission denied: lock held") + + fake = _HeldMsvcrt() + monkeypatch.setattr(module, "msvcrt", fake) + path = tmp_path / "state.db" + _maintenance_schema(path) + lock = tmp_path / "turns.lock" + + with pytest.raises(PermissionError): + module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) + + assert fake.calls == [(fake.LK_NBLCK, 0, 1, 1)] + with closing(sqlite3.connect(str(path))) as conn: + assert conn.execute("PRAGMA index_list(messages)").fetchall() == [] From f04e3ca8b2fade9ecec90a96c69d6277ffda49b7 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Thu, 17 Sep 2026 22:59:02 +0000 Subject: [PATCH 13/57] fix(sqlite): write the Windows lock byte in binary; drop source-text asserts On native Windows the text-mode ``handle.write("\n")`` that seeds an empty ``--lock-file`` is translated to ``\r\n``, so a fresh lock file is two bytes and the msvcrt regression test fails on the platform it protects. Seed the file with ``os.write(fd, b"\0")`` on the descriptor instead: exactly one byte whatever the text-mode newline translation. The Windows-branch tests now run under a ``\n`` -> ``\r\n`` translating ``open`` so this failure reproduces on any host. Remove the two ``... not in src`` assertions added for the previous review from test_issue2628: the behaviours they hinted at (every missing-index projection uses the pre-aggregated ordering; listing never creates an index) are already asserted through query plans, result equivalence and ``PRAGMA index_list`` in test_pr7445_readonly_review_followups.py, test_state_db_readonly_readers.py and test_issue3887_index_prime.py. --- scripts/ensure_state_db_read_indexes.py | 6 +++-- tests/test_issue2628_cli_sessions_perf.py | 4 ---- .../test_pr7445_readonly_review_followups.py | 24 +++++++++++++++---- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/scripts/ensure_state_db_read_indexes.py b/scripts/ensure_state_db_read_indexes.py index 2eedc9a4..31dce342 100644 --- a/scripts/ensure_state_db_read_indexes.py +++ b/scripts/ensure_state_db_read_indexes.py @@ -67,9 +67,11 @@ def _exclusive_lock(lock_file): # Lock one byte at offset 0 without blocking; raises OSError when held. # ``a+`` may have just created an empty file, and Windows cannot lock # a range beyond EOF, so make sure byte 0 exists first. An existing - # (deployment-owned) lock file is left untouched. + # (deployment-owned) lock file is left untouched. Write the byte on + # the descriptor: a text-mode ``handle.write("\n")`` becomes two + # bytes (``\r\n``) on native Windows. if os.fstat(handle.fileno()).st_size == 0: - handle.write("\n") + os.write(handle.fileno(), b"\0") handle.flush() handle.seek(0) msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) diff --git a/tests/test_issue2628_cli_sessions_perf.py b/tests/test_issue2628_cli_sessions_perf.py index 556e5509..950a6028 100644 --- a/tests/test_issue2628_cli_sessions_perf.py +++ b/tests/test_issue2628_cli_sessions_perf.py @@ -209,12 +209,8 @@ def test_importable_agent_rows_push_sidebar_limit_into_sql(tmp_path): assert "JOIN candidates c ON c.id = s.id" in src assert "latest_messages AS" in src assert "LEFT JOIN latest_messages lm ON lm.session_id = s.id" in src - # Pre-aggregation covers every missing-index projection, not only cron (#7445 review). - assert 'included == ("cron",)' not in src assert "not messages_index_present" in src assert "PRAGMA index_list(messages)" in src - # Listing is a pure read: index creation lives in the drained maintenance tool. - assert "CREATE INDEX IF NOT EXISTS idx_messages_session" not in src assert "_CRON_PREAGGREGATE_CANDIDATE_ORDER_MIN_MESSAGES" not in src assert "MAX(mx.timestamp) FROM messages mx WHERE mx.session_id = s.id" in src assert "candidate_limit = max(result_limit * 8, result_limit)" in src diff --git a/tests/test_pr7445_readonly_review_followups.py b/tests/test_pr7445_readonly_review_followups.py index f99c1548..4239afaa 100644 --- a/tests/test_pr7445_readonly_review_followups.py +++ b/tests/test_pr7445_readonly_review_followups.py @@ -18,7 +18,9 @@ Contract under test: there with a clear error instead of an ``ImportError`` at import time; * on the ``msvcrt`` branch the tool locks byte 0 of the lock file only after making sure that byte exists, so a fresh (empty) ``--lock-file`` is acquired - without relying on the CRT's beyond-EOF locking behaviour. + without relying on the CRT's beyond-EOF locking behaviour; that byte is + written as exactly one byte even under the ``\\n`` → ``\\r\\n`` translation + native Windows applies to text-mode handles. """ import importlib import os @@ -291,7 +293,20 @@ class _StrictMsvcrt: raise OSError(36, "Resource deadlock avoided: lock region beyond end of file") -def test_windows_lock_branch_acquires_a_fresh_empty_lock_file(tmp_path, maintenance_without_fcntl, monkeypatch): +@pytest.fixture +def windows_text_mode(maintenance_without_fcntl, monkeypatch): + """Make the tool's text-mode ``open`` translate ``\\n`` to ``\\r\\n`` as on native Windows.""" + real_open = open + + def crlf_text_open(file, mode="r", *args, **kwargs): + if "b" not in mode: + kwargs.setdefault("newline", "\r\n") + return real_open(file, mode, *args, **kwargs) + + monkeypatch.setattr(maintenance_without_fcntl, "open", crlf_text_open, raising=False) + + +def test_windows_lock_branch_acquires_a_fresh_empty_lock_file(tmp_path, maintenance_without_fcntl, windows_text_mode, monkeypatch): module = maintenance_without_fcntl assert module.fcntl is None # the fixture already routes the tool to the msvcrt branch fake = _StrictMsvcrt() @@ -305,7 +320,8 @@ def test_windows_lock_branch_acquires_a_fresh_empty_lock_file(tmp_path, maintena result = module.ensure_read_indexes(path, confirmed_drained=True, lock_file=lock) assert set(result.values()) == {"created"} - # Exactly one non-blocking lock on byte 0, taken while that byte existed. + # Exactly one non-blocking lock on byte 0, taken while that byte existed + # and was the only one: no newline translation may inflate the file. assert fake.calls == [(fake.LK_NBLCK, 0, 1, 1)] assert lock.stat().st_size == 1 with closing(sqlite3.connect(str(path))) as conn: @@ -329,7 +345,7 @@ def test_windows_lock_branch_keeps_an_existing_lock_file_intact(tmp_path, mainte assert lock.read_bytes() == b"deployment-owned\n" -def test_windows_lock_branch_reports_a_held_lock_without_touching_the_db(tmp_path, maintenance_without_fcntl, monkeypatch): +def test_windows_lock_branch_reports_a_held_lock_without_touching_the_db(tmp_path, maintenance_without_fcntl, windows_text_mode, monkeypatch): module = maintenance_without_fcntl class _HeldMsvcrt(_StrictMsvcrt): From c748049bf7291737caa1d3e9fe1fc555cc9b2f1b Mon Sep 17 00:00:00 2001 From: James Drake Date: Tue, 11 Aug 2026 23:30:08 -0500 Subject: [PATCH 14/57] fix: clean up failed chat worker launches --- api/routes.py | 25 +++++ .../test_issue6869_gateway_launch_failure.py | 93 +++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 tests/test_issue6869_gateway_launch_failure.py diff --git a/api/routes.py b/api/routes.py index 7d30ab90..b73ec9af 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22810,6 +22810,30 @@ def _prepare_chat_start_session_for_stream( s.save() +def _cleanup_chat_start_launch_failure(session, stream_id: str) -> None: + """Release state registered before a worker thread successfully starts.""" + clear_session_writeback_owner_if_owned(session.session_id, stream_id) + unregister_stream_owner(stream_id) + with STREAMS_LOCK: + STREAMS.pop(stream_id, None) + STREAM_GOAL_RELATED.pop(stream_id, None) + if getattr(session, "active_stream_id", None) != stream_id: + return + session.active_stream_id = None + session.pending_user_message = None + session.pending_attachments = [] + session.pending_started_at = None + session.pending_user_source = None + try: + session.save() + except Exception: + logger.debug( + "Failed to persist chat-start cleanup after worker launch failure for %s", + stream_id, + exc_info=True, + ) + + def _is_hidden_empty_session(s) -> bool: return ( getattr(s, "title", "Untitled") == "Untitled" @@ -23393,6 +23417,7 @@ def _start_chat_stream_for_session( _clear_gateway_run_starting(stream_id) except Exception: logger.debug("Failed to record gateway run-start failure for stream %s", stream_id, exc_info=True) + _cleanup_chat_start_launch_failure(s, stream_id) raise response = { "stream_id": stream_id, diff --git a/tests/test_issue6869_gateway_launch_failure.py b/tests/test_issue6869_gateway_launch_failure.py new file mode 100644 index 00000000..7d8c6fe0 --- /dev/null +++ b/tests/test_issue6869_gateway_launch_failure.py @@ -0,0 +1,93 @@ +"""Regression tests for #6869: failed Gateway worker launch cleanup.""" + +from types import SimpleNamespace + +import api.config as config +import api.routes as routes +from api import turn_journal + + +def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state(monkeypatch): + """A thread-start exception must not leave one owner per failed Gateway launch.""" + class FailingThread: + def __init__(self, *args, **kwargs): + pass + + def start(self): + raise RuntimeError("thread launch failed") + + def fake_prepare(session, *, stream_id, **kwargs): + session.active_stream_id = stream_id + session.pending_user_message = kwargs["msg"] + session.pending_started_at = 1.0 + config.register_session_writeback_owner(session.session_id, stream_id) + + monkeypatch.setattr(routes.threading, "Thread", FailingThread) + monkeypatch.setattr(routes, "_prepare_chat_start_session_for_stream", fake_prepare) + monkeypatch.setattr(routes, "set_last_workspace", lambda _workspace: None) + monkeypatch.setattr(routes, "_is_hidden_empty_session", lambda _session: False) + monkeypatch.setattr( + turn_journal, + "append_turn_journal_event", + lambda *_args, **_kwargs: {}, + ) + + for index in range(3): + session = SimpleNamespace( + session_id=f"session-launch-failure-{index}", + title="Existing", + active_stream_id=None, + pending_user_message=None, + pending_attachments=[], + pending_started_at=None, + pending_user_source=None, + messages=[{"role": "user", "content": "old"}], + workspace="/tmp", + model="old-model", + model_provider=None, + worktree_path=None, + profile=None, + save=lambda: None, + ) + + try: + routes._start_chat_stream_for_session( + session, + msg="start gateway turn", + attachments=[], + workspace="/tmp", + model="test-model", + external_runtime_owned=True, + ) + except RuntimeError as exc: + assert str(exc) == "thread launch failed" + else: + raise AssertionError("thread-start failure must propagate") + + assert config.SESSION_WRITEBACK_OWNERS == {} + assert config.STREAM_SESSION_OWNERS == {} + assert config.STREAMS == {} + + +def test_gateway_launch_failure_cleanup_does_not_clear_successor_owner(): + """Cleanup must be compare-and-clear when a successor already took over.""" + session = SimpleNamespace( + session_id="session-launch-successor", + active_stream_id="new-stream", + pending_user_message="new prompt", + pending_attachments=["new.txt"], + pending_started_at=2.0, + pending_user_source="webui", + save=lambda: None, + ) + config.register_session_writeback_owner(session.session_id, "old-stream") + config.register_session_writeback_owner(session.session_id, "new-stream") + config.register_stream_owner("old-stream", session.session_id) + config.STREAMS["old-stream"] = object() + + routes._cleanup_chat_start_launch_failure(session, "old-stream") + + assert config.session_writeback_owner(session.session_id) == "new-stream" + assert session.active_stream_id == "new-stream" + assert session.pending_user_message == "new prompt" + assert "old-stream" not in config.STREAMS From b5c0136545a2ed65aeb98163a954f2275721d452 Mon Sep 17 00:00:00 2001 From: James Drake Date: Thu, 27 Aug 2026 08:34:52 -0500 Subject: [PATCH 15/57] fix: run launch-failure session reset under session lock The session-field half of _cleanup_chat_start_launch_failure mutated and saved the passed-in session object without holding the per-session lock and without re-resolving the canonical session. A concurrent successor turn (same session_id, new stream_id) or a delete could be clobbered: pending fields wiped or a deleted session resurrected via save(). Mirror the established #1533 pattern (routes.py:3077) and anchor-scene write guard (routes.py:5140): hold _get_session_agent_lock, re-resolve with get_session(), early-return on KeyError (session deleted) or when the canonical no longer points at the failed stream, then reset pending fields on the canonical object only. Add deterministic race coverage: successor admitted during the check-then-clear window keeps its pending fields and is not saved away, and a deleted session is not resurrected by save(). Both tests fail against the previous implementation. --- api/routes.py | 42 +++-- .../test_issue6869_gateway_launch_failure.py | 150 ++++++++++++++---- 2 files changed, 144 insertions(+), 48 deletions(-) diff --git a/api/routes.py b/api/routes.py index b73ec9af..da9537b3 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22817,21 +22817,33 @@ def _cleanup_chat_start_launch_failure(session, stream_id: str) -> None: with STREAMS_LOCK: STREAMS.pop(stream_id, None) STREAM_GOAL_RELATED.pop(stream_id, None) - if getattr(session, "active_stream_id", None) != stream_id: - return - session.active_stream_id = None - session.pending_user_message = None - session.pending_attachments = [] - session.pending_started_at = None - session.pending_user_source = None - try: - session.save() - except Exception: - logger.debug( - "Failed to persist chat-start cleanup after worker launch failure for %s", - stream_id, - exc_info=True, - ) + # The session-field reset needs the same concurrency discipline as the + # registry half: hold the per-session lock and re-resolve the canonical + # session before clearing anything. Mutating the passed-in stale object + # could wipe a concurrent successor turn's pending fields, and saving it + # could resurrect a session deleted while the launch was failing. Same + # pattern as the #1533 race fix (routes.py:3077) and the anchor-scene + # write guard (routes.py:5140). + with _get_session_agent_lock(session.session_id): + try: + canonical = get_session(session.session_id) + except KeyError: + return # session deleted while the thread launch was failing + if getattr(canonical, "active_stream_id", None) != stream_id: + return # a successor turn already owns the session + canonical.active_stream_id = None + canonical.pending_user_message = None + canonical.pending_attachments = [] + canonical.pending_started_at = None + canonical.pending_user_source = None + try: + canonical.save() + except Exception: + logger.debug( + "Failed to persist chat-start cleanup after worker launch failure for %s", + stream_id, + exc_info=True, + ) def _is_hidden_empty_session(s) -> bool: diff --git a/tests/test_issue6869_gateway_launch_failure.py b/tests/test_issue6869_gateway_launch_failure.py index 7d8c6fe0..703209b9 100644 --- a/tests/test_issue6869_gateway_launch_failure.py +++ b/tests/test_issue6869_gateway_launch_failure.py @@ -7,6 +7,32 @@ import api.routes as routes from api import turn_journal +def _make_session(session_id, *, active_stream_id=None, pending_user_message=None, save=None): + return SimpleNamespace( + session_id=session_id, + title="Existing", + active_stream_id=active_stream_id, + pending_user_message=pending_user_message, + pending_attachments=[], + pending_started_at=None, + pending_user_source=None, + messages=[{"role": "user", "content": "old"}], + workspace="/tmp", + model="old-model", + model_provider=None, + worktree_path=None, + profile=None, + save=save or (lambda: None), + ) + + +def _register_failed_stream(session_id, stream_id): + """Mirror the registry state _prepare_chat_start_session_for_stream installs.""" + config.register_session_writeback_owner(session_id, stream_id) + config.register_stream_owner(stream_id, session_id) + config.STREAMS[stream_id] = object() + + def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state(monkeypatch): """A thread-start exception must not leave one owner per failed Gateway launch.""" class FailingThread: @@ -16,16 +42,24 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( def start(self): raise RuntimeError("thread launch failed") + canonical_by_id = {} + def fake_prepare(session, *, stream_id, **kwargs): session.active_stream_id = stream_id session.pending_user_message = kwargs["msg"] session.pending_started_at = 1.0 - config.register_session_writeback_owner(session.session_id, stream_id) + _register_failed_stream(session.session_id, stream_id) + canonical_by_id[session.session_id] = session monkeypatch.setattr(routes.threading, "Thread", FailingThread) monkeypatch.setattr(routes, "_prepare_chat_start_session_for_stream", fake_prepare) monkeypatch.setattr(routes, "set_last_workspace", lambda _workspace: None) monkeypatch.setattr(routes, "_is_hidden_empty_session", lambda _session: False) + monkeypatch.setattr( + routes, + "get_session", + lambda sid, metadata_only=False: canonical_by_id[sid], + ) monkeypatch.setattr( turn_journal, "append_turn_journal_event", @@ -33,23 +67,7 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( ) for index in range(3): - session = SimpleNamespace( - session_id=f"session-launch-failure-{index}", - title="Existing", - active_stream_id=None, - pending_user_message=None, - pending_attachments=[], - pending_started_at=None, - pending_user_source=None, - messages=[{"role": "user", "content": "old"}], - workspace="/tmp", - model="old-model", - model_provider=None, - worktree_path=None, - profile=None, - save=lambda: None, - ) - + session = _make_session(f"session-launch-failure-{index}") try: routes._start_chat_stream_for_session( session, @@ -63,31 +81,97 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( assert str(exc) == "thread launch failed" else: raise AssertionError("thread-start failure must propagate") + assert session.active_stream_id is None + assert session.pending_user_message is None + assert session.pending_started_at is None assert config.SESSION_WRITEBACK_OWNERS == {} assert config.STREAM_SESSION_OWNERS == {} assert config.STREAMS == {} -def test_gateway_launch_failure_cleanup_does_not_clear_successor_owner(): - """Cleanup must be compare-and-clear when a successor already took over.""" - session = SimpleNamespace( - session_id="session-launch-successor", +def test_gateway_launch_failure_cleanup_does_not_clear_successor_owner(monkeypatch): + """A successor fully installed before cleanup keeps its registry ownership.""" + canonical = _make_session( + "session-launch-successor", active_stream_id="new-stream", pending_user_message="new prompt", - pending_attachments=["new.txt"], - pending_started_at=2.0, - pending_user_source="webui", save=lambda: None, ) - config.register_session_writeback_owner(session.session_id, "old-stream") - config.register_session_writeback_owner(session.session_id, "new-stream") - config.register_stream_owner("old-stream", session.session_id) - config.STREAMS["old-stream"] = object() + monkeypatch.setattr( + routes, + "get_session", + lambda sid, metadata_only=False: canonical, + ) + stale = _make_session("session-launch-successor") + _register_failed_stream(stale.session_id, "old-stream") + config.register_session_writeback_owner(stale.session_id, "new-stream") - routes._cleanup_chat_start_launch_failure(session, "old-stream") + routes._cleanup_chat_start_launch_failure(stale, "old-stream") - assert config.session_writeback_owner(session.session_id) == "new-stream" - assert session.active_stream_id == "new-stream" - assert session.pending_user_message == "new prompt" + assert config.session_writeback_owner(stale.session_id) == "new-stream" + assert canonical.active_stream_id == "new-stream" + assert canonical.pending_user_message == "new prompt" assert "old-stream" not in config.STREAMS + + +def test_gateway_launch_failure_cleanup_does_not_wipe_successor_admitted_during_cleanup(monkeypatch): + """A successor admitted mid-cleanup survives: the guard runs on the re-resolved + canonical session, not on the stale passed-in object that still shows the + failed stream.""" + saved = [] + + canonical = _make_session( + "session-launch-race", + active_stream_id="new-stream", + pending_user_message="successor prompt", + save=lambda: saved.append("canonical"), + ) + monkeypatch.setattr( + routes, + "get_session", + lambda sid, metadata_only=False: canonical, + ) + stale = _make_session( + "session-launch-race", + active_stream_id="old-stream", + pending_user_message="failed-stream prompt", + save=lambda: saved.append("stale"), + ) + _register_failed_stream(stale.session_id, "old-stream") + config.register_session_writeback_owner(stale.session_id, "new-stream") + + routes._cleanup_chat_start_launch_failure(stale, "old-stream") + + assert config.session_writeback_owner(stale.session_id) == "new-stream" + assert canonical.active_stream_id == "new-stream" + assert canonical.pending_user_message == "successor prompt" + assert "old-stream" not in config.STREAMS + assert saved == [], "cleanup must not save a session it early-returns on" + + +def test_gateway_launch_failure_cleanup_does_not_resurrect_deleted_session(monkeypatch): + """A session deleted while the launch was failing must not be recreated: + the resolver raises KeyError and cleanup returns without saving anything.""" + saved = [] + + def deleted_resolver(sid, metadata_only=False): + raise KeyError(sid) + + monkeypatch.setattr(routes, "get_session", deleted_resolver) + stale = _make_session( + "session-launch-deleted", + active_stream_id="old-stream", + pending_user_message="pending prompt", + save=lambda: saved.append("stale"), + ) + _register_failed_stream(stale.session_id, "old-stream") + + routes._cleanup_chat_start_launch_failure(stale, "old-stream") + + assert config.session_writeback_owner(stale.session_id) is None + assert "old-stream" not in config.STREAM_SESSION_OWNERS + assert "old-stream" not in config.STREAMS + assert stale.active_stream_id == "old-stream" + assert stale.pending_user_message == "pending prompt" + assert saved == [], "cleanup must not save a deleted session" \ No newline at end of file From f444cd1d2e0eeb8f9d5a516a4a308f34d583f9d5 Mon Sep 17 00:00:00 2001 From: James Drake Date: Fri, 18 Sep 2026 08:23:29 -0500 Subject: [PATCH 16/57] test: accept the profile kwarg in the launch-failure stub Master added a profile= kwarg to set_last_workspace; the monkeypatched stub in this test took only the workspace arg and broke on the rebase. --- tests/test_issue6869_gateway_launch_failure.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_issue6869_gateway_launch_failure.py b/tests/test_issue6869_gateway_launch_failure.py index 703209b9..fdb731bb 100644 --- a/tests/test_issue6869_gateway_launch_failure.py +++ b/tests/test_issue6869_gateway_launch_failure.py @@ -53,7 +53,7 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( monkeypatch.setattr(routes.threading, "Thread", FailingThread) monkeypatch.setattr(routes, "_prepare_chat_start_session_for_stream", fake_prepare) - monkeypatch.setattr(routes, "set_last_workspace", lambda _workspace: None) + monkeypatch.setattr(routes, "set_last_workspace", lambda _workspace, **_kw: None) monkeypatch.setattr(routes, "_is_hidden_empty_session", lambda _session: False) monkeypatch.setattr( routes, From 218164f6c889fa3e1cf53b2bad41492933e5361d Mon Sep 17 00:00:00 2001 From: James Drake Date: Fri, 18 Sep 2026 08:28:03 -0500 Subject: [PATCH 17/57] test: scope the launch-failure registry asserts to this test's own streams scripts/test.sh runs the whole selection in one process (no per-file subprocess isolation), so asserting the global registries are empty made this test fail on unrelated files that leave entries behind. --- tests/test_issue6869_gateway_launch_failure.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/test_issue6869_gateway_launch_failure.py b/tests/test_issue6869_gateway_launch_failure.py index fdb731bb..d4ea0ea0 100644 --- a/tests/test_issue6869_gateway_launch_failure.py +++ b/tests/test_issue6869_gateway_launch_failure.py @@ -43,6 +43,7 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( raise RuntimeError("thread launch failed") canonical_by_id = {} + registered_stream_ids = [] def fake_prepare(session, *, stream_id, **kwargs): session.active_stream_id = stream_id @@ -50,6 +51,7 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( session.pending_started_at = 1.0 _register_failed_stream(session.session_id, stream_id) canonical_by_id[session.session_id] = session + registered_stream_ids.append(stream_id) monkeypatch.setattr(routes.threading, "Thread", FailingThread) monkeypatch.setattr(routes, "_prepare_chat_start_session_for_stream", fake_prepare) @@ -85,9 +87,15 @@ def test_gateway_thread_start_failure_releases_writeback_owner_and_stream_state( assert session.pending_user_message is None assert session.pending_started_at is None - assert config.SESSION_WRITEBACK_OWNERS == {} - assert config.STREAM_SESSION_OWNERS == {} - assert config.STREAMS == {} + # Scope these to the streams this test registered. scripts/test.sh runs the + # whole selection in one process, so asserting the global registries are + # empty makes this test fail on unrelated files that leave entries behind. + assert registered_stream_ids, "the launch path must register state before failing" + for stream_id in registered_stream_ids: + assert stream_id not in config.STREAM_SESSION_OWNERS + assert stream_id not in config.STREAMS + for session_id in canonical_by_id: + assert config.session_writeback_owner(session_id) is None def test_gateway_launch_failure_cleanup_does_not_clear_successor_owner(monkeypatch): From 11faed37683ba08839a5226f6439524dc5b24d44 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Fri, 18 Sep 2026 23:40:27 +0000 Subject: [PATCH 18/57] test(sqlite): assert the capped candidate window behaviourally, not via source text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the remaining source-text assertions in test_issue2628 (the Greptile P2 thread): the pre-aggregation shape, index detection, correlated seek and pure-read contracts are already pinned behaviourally by EXPLAIN QUERY PLAN, result equivalence and PRAGMA index_list in test_pr7445_readonly_review_followups.py, test_state_db_readonly_readers.py and test_issue3887_index_prime.py. The one contract only source text covered — the SQL candidate window is capped and oversampled (limit * 8) so post-projection filtering cannot starve the visible page — is now asserted behaviourally instead: * the traced statement SQLite actually runs carries a single LIMIT whose bound is the oversampled cap (limit * 8 = 160), applied inside the candidate subquery the outer projection joins; * a page still fills to limit when the 30 newest candidates are invisible CLI rows (ended, default title): they consume candidates, not result slots. Verified to fail (0 rows) against the old non-oversampled shape. --- tests/test_issue2628_cli_sessions_perf.py | 80 +++++++++++++++++++---- 1 file changed, 67 insertions(+), 13 deletions(-) diff --git a/tests/test_issue2628_cli_sessions_perf.py b/tests/test_issue2628_cli_sessions_perf.py index 950a6028..6f31f031 100644 --- a/tests/test_issue2628_cli_sessions_perf.py +++ b/tests/test_issue2628_cli_sessions_perf.py @@ -1,15 +1,14 @@ """Regression coverage for capped CLI/agent session sidebar scans (#2628).""" -import pathlib import sqlite3 import time +from contextlib import contextmanager import pytest import api.agent_sessions as agent_sessions _REAL_SQLITE_CONNECT = sqlite3.connect -REPO_ROOT = pathlib.Path(__file__).resolve().parents[1] def _make_state_db(path, *, sessions=80, messages_per_session=3, create_messages_index=True, source="cli", session_source="cli"): @@ -169,6 +168,24 @@ def _make_connect_with_progress_budget(*, budget_ops, interval=1): return _connect +@contextmanager +def _capture_listing_sql(): + """Record the SQL statements the listing runs (trace callback, read-only).""" + statements = [] + + def _tracing_connect(*args, **kwargs): + conn = _REAL_SQLITE_CONNECT(*args, **kwargs) + conn.set_trace_callback(statements.append) + return conn + + original_connect = agent_sessions.sqlite3.connect + agent_sessions.sqlite3.connect = _tracing_connect + try: + yield statements + finally: + agent_sessions.sqlite3.connect = original_connect + + def _make_connect_with_progress_counter(*, interval=1): steps = {"count": 0} @@ -198,22 +215,59 @@ def test_importable_agent_rows_push_sidebar_limit_into_sql(tmp_path): db = tmp_path / "state.db" _make_state_db(db, sessions=120, messages_per_session=5) - rows = agent_sessions.read_importable_agent_session_rows(db, limit=20, exclude_sources=("webui",)) + with _capture_listing_sql() as statements: + rows = agent_sessions.read_importable_agent_session_rows(db, limit=20, exclude_sources=("webui",)) assert len(rows) == 20 assert [row["id"] for row in rows][:3] == ["cli_perf_0119", "cli_perf_0118", "cli_perf_0117"] assert {row["actual_message_count"] for row in rows} == {5} - src = (REPO_ROOT / "api" / "agent_sessions.py").read_text() - assert "WITH candidates AS" in src - assert "JOIN candidates c ON c.id = s.id" in src - assert "latest_messages AS" in src - assert "LEFT JOIN latest_messages lm ON lm.session_id = s.id" in src - assert "not messages_index_present" in src - assert "PRAGMA index_list(messages)" in src - assert "_CRON_PREAGGREGATE_CANDIDATE_ORDER_MIN_MESSAGES" not in src - assert "MAX(mx.timestamp) FROM messages mx WHERE mx.session_id = s.id" in src - assert "candidate_limit = max(result_limit * 8, result_limit)" in src + candidates_sql = next( + statement + for statement in statements + if "LIMIT" in statement and "FROM sessions s" in statement + ) + # The cap is applied inside the candidate subquery, not in Python after an + # unbounded aggregate: the oversampled window (limit * 8 = 160 here) is + # part of the statement SQLite actually runs, and it bounds the candidate + # subquery that the outer projection joins. + assert candidates_sql.count("LIMIT") == 1, candidates_sql + window_end = candidates_sql.index("LIMIT") + window = candidates_sql[:window_end] + assert "JOIN candidates" not in window, candidates_sql + cap = int(candidates_sql[window_end:].split(None, 2)[1]) + assert cap == 160, candidates_sql + + +def test_importable_agent_rows_candidate_window_overfilters_then_fills(tmp_path): + """Rows dropped after projection must not shrink the returned page. + + The SQL candidate window is oversampled (limit * 8) precisely so sessions + filtered client-side of the SQL (invisible CLI rows here) only consume + candidates, not result slots: a page that would be short if the cap were + the plain ``limit`` still fills to ``limit`` visible rows. + """ + db = tmp_path / "state.db" + _make_state_db(db, sessions=120, messages_per_session=5) + + # 30 newest sessions are CLI rows that the projection hides (ended, default + # title, no user turn visible): they must fall out of the window without + # starving the visible page below them. + conn = sqlite3.connect(str(db)) + for i in range(90, 120): + conn.execute( + "UPDATE sessions SET title = 'cli session', ended_at = 1.0, end_reason = 'timeout' WHERE id = ?", + (f"cli_perf_{i:04d}",), + ) + conn.commit() + conn.close() + + rows = agent_sessions.read_importable_agent_session_rows(db, limit=20, exclude_sources=("webui",)) + + assert len(rows) == 20 + assert rows[0]["id"] == "cli_perf_0089" + assert rows[-1]["id"] == "cli_perf_0070" + assert {row["actual_message_count"] for row in rows} == {5} def test_importable_agent_rows_candidate_ordering_stays_under_progress_budget(tmp_path, monkeypatch): From 047dfa40a54ad5d3957ab76cdaf013edd2b4b50e Mon Sep 17 00:00:00 2001 From: James Drake Date: Sat, 19 Sep 2026 00:29:07 -0500 Subject: [PATCH 19/57] fix(routes): never mask the launch error during cleanup _cleanup_chat_start_launch_failure runs inside the launch-failure handler, but the session re-resolve only caught KeyError. An unreadable or undeserializable session store raises something else, and that escaped the helper, replacing the worker-launch error the caller is about to report and aborting the reset after the registries were already cleared. Contain the resolution-and-reset so the helper cannot raise. The KeyError branch keeps its meaning (deleted session, return without saving); anything else is logged at debug like the existing save() failure path. --- api/routes.py | 52 ++++++++++++------- .../test_issue6869_gateway_launch_failure.py | 34 +++++++++++- 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/api/routes.py b/api/routes.py index da9537b3..e8054150 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22824,26 +22824,38 @@ def _cleanup_chat_start_launch_failure(session, stream_id: str) -> None: # could resurrect a session deleted while the launch was failing. Same # pattern as the #1533 race fix (routes.py:3077) and the anchor-scene # write guard (routes.py:5140). - with _get_session_agent_lock(session.session_id): - try: - canonical = get_session(session.session_id) - except KeyError: - return # session deleted while the thread launch was failing - if getattr(canonical, "active_stream_id", None) != stream_id: - return # a successor turn already owns the session - canonical.active_stream_id = None - canonical.pending_user_message = None - canonical.pending_attachments = [] - canonical.pending_started_at = None - canonical.pending_user_source = None - try: - canonical.save() - except Exception: - logger.debug( - "Failed to persist chat-start cleanup after worker launch failure for %s", - stream_id, - exc_info=True, - ) + # + # This runs while the original launch failure is being handled, so it must + # never raise. Lock acquisition and session resolution can fail on their own + # (I/O, deserialization), and an escaping error here would mask the launch + # failure the caller is about to report while leaving the reset half done. + try: + with _get_session_agent_lock(session.session_id): + try: + canonical = get_session(session.session_id) + except KeyError: + return # session deleted while the thread launch was failing + if getattr(canonical, "active_stream_id", None) != stream_id: + return # a successor turn already owns the session + canonical.active_stream_id = None + canonical.pending_user_message = None + canonical.pending_attachments = [] + canonical.pending_started_at = None + canonical.pending_user_source = None + try: + canonical.save() + except Exception: + logger.debug( + "Failed to persist chat-start cleanup after worker launch failure for %s", + stream_id, + exc_info=True, + ) + except Exception: + logger.debug( + "Failed to reset session state after worker launch failure for %s", + stream_id, + exc_info=True, + ) def _is_hidden_empty_session(s) -> bool: diff --git a/tests/test_issue6869_gateway_launch_failure.py b/tests/test_issue6869_gateway_launch_failure.py index d4ea0ea0..01563067 100644 --- a/tests/test_issue6869_gateway_launch_failure.py +++ b/tests/test_issue6869_gateway_launch_failure.py @@ -182,4 +182,36 @@ def test_gateway_launch_failure_cleanup_does_not_resurrect_deleted_session(monke assert "old-stream" not in config.STREAMS assert stale.active_stream_id == "old-stream" assert stale.pending_user_message == "pending prompt" - assert saved == [], "cleanup must not save a deleted session" \ No newline at end of file + assert saved == [], "cleanup must not save a deleted session" + + +def test_gateway_launch_failure_cleanup_never_masks_the_launch_error(monkeypatch): + """A resolution failure during cleanup must not replace the launch error. + + Cleanup runs inside the launch-failure handler, so an error it fails to + contain is reported instead of the real launch failure. Resolution can fail + with something other than KeyError when the session store is unreadable or + cannot be deserialized. + """ + saved = [] + + def failing_resolver(sid, metadata_only=False): + raise OSError("session store unreadable") + + monkeypatch.setattr(routes, "get_session", failing_resolver) + stale = _make_session( + "session-launch-resolution-error", + active_stream_id="old-stream", + pending_user_message="pending prompt", + save=lambda: saved.append("stale"), + ) + _register_failed_stream(stale.session_id, "old-stream") + + # Must not raise: the caller is already handling the launch failure. + routes._cleanup_chat_start_launch_failure(stale, "old-stream") + + # The registry half still completed, and nothing was saved. + assert config.session_writeback_owner(stale.session_id) is None + assert "old-stream" not in config.STREAM_SESSION_OWNERS + assert "old-stream" not in config.STREAMS + assert saved == [] \ No newline at end of file From 63d024dc45156d1ab0067a135dab776ce5dd6328 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Fri, 18 Sep 2026 11:40:51 +0000 Subject: [PATCH 20/57] feat(sidebar): make the visible-session window size configurable CLI_VISIBLE_SESSION_LIMIT is hardcoded to 20. That constant also bounds how many delegated subagent children can render at once, since a child only nests when its parent row is in the same payload, so operators running wide delegate_task fan-outs cannot see all children without editing the source. Read it from HERMES_WEBUI_VISIBLE_SESSION_LIMIT via the existing _env_int helper, keeping 20 as the default. --- api/models.py | 6 ++++- tests/test_visible_session_limit_env.py | 35 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 tests/test_visible_session_limit_env.py diff --git a/api/models.py b/api/models.py index 84a91b9c..99226cb4 100644 --- a/api/models.py +++ b/api/models.py @@ -48,7 +48,11 @@ from api.agent_sessions import ( from api.process_event_utils import stamp_message_source logger = logging.getLogger(__name__) -CLI_VISIBLE_SESSION_LIMIT = 20 +# Size of the interactive sidebar recency window. Also bounds how many +# delegated subagent children can be rendered at once, since a child only +# nests when it wins a slot in this window. Override with +# HERMES_WEBUI_VISIBLE_SESSION_LIMIT. +CLI_VISIBLE_SESSION_LIMIT = _cfg._env_int("HERMES_WEBUI_VISIBLE_SESSION_LIMIT", 20) # How many messageful cron sessions to surface in the project-chip layer. # Needs to exceed CLI_VISIBLE_SESSION_LIMIT so older cron runs stay # addressable even when many newer non-cron sessions dominate the default diff --git a/tests/test_visible_session_limit_env.py b/tests/test_visible_session_limit_env.py new file mode 100644 index 00000000..178465f5 --- /dev/null +++ b/tests/test_visible_session_limit_env.py @@ -0,0 +1,35 @@ +"""CLI_VISIBLE_SESSION_LIMIT is overridable via HERMES_WEBUI_VISIBLE_SESSION_LIMIT. + +The sidebar recency window also bounds how many delegated subagent children can +render at once, since a child only nests when its row wins a slot in the same +payload. Operators running wide fan-outs need to raise it without editing code. +""" + +import importlib + + +def _reload_limit(monkeypatch, value): + if value is None: + monkeypatch.delenv("HERMES_WEBUI_VISIBLE_SESSION_LIMIT", raising=False) + else: + monkeypatch.setenv("HERMES_WEBUI_VISIBLE_SESSION_LIMIT", value) + import api.models as models + + return importlib.reload(models).CLI_VISIBLE_SESSION_LIMIT + + +def test_defaults_to_20_when_unset(monkeypatch): + assert _reload_limit(monkeypatch, None) == 20 + + +def test_env_override_raises_the_window(monkeypatch): + assert _reload_limit(monkeypatch, "64") == 64 + + +def test_invalid_value_falls_back_to_default(monkeypatch): + assert _reload_limit(monkeypatch, "bogus") == 20 + + +def test_zero_and_negative_fall_back_to_default(monkeypatch): + assert _reload_limit(monkeypatch, "0") == 20 + assert _reload_limit(monkeypatch, "-5") == 20 From 899b89e1d3456de78a3bf92efd03158230bc4553 Mon Sep 17 00:00:00 2001 From: carlotestor Date: Fri, 18 Sep 2026 18:39:47 +0000 Subject: [PATCH 21/57] docs+test: document HERMES_WEBUI_VISIBLE_SESSION_LIMIT; probe it in a subprocess - README env table + .env.example gain the new setting (default 20, fallback semantics), pinned by test_setting_is_documented. - tests/test_visible_session_limit_env.py no longer importlib.reload()s api.models in the shared pytest process (which recreated its module-level locks/caches/classes behind other modules' backs); each case now imports api.models in a fresh interpreter with a scrubbed env, matching test_issue3283_profiles_config_import_order. --- .env.example | 5 ++ README.md | 1 + tests/test_visible_session_limit_env.py | 72 ++++++++++++++++++------- 3 files changed, 60 insertions(+), 18 deletions(-) diff --git a/.env.example b/.env.example index 3a3d4c13..2682308f 100644 --- a/.env.example +++ b/.env.example @@ -132,6 +132,11 @@ # Extra sources for the notes drawer (see docs for the expected format). # HERMES_WEBUI_EXTERNAL_NOTES_SOURCES= +# Sidebar recency window: how many recent sessions are listed (default: 20). +# Also caps how many delegated subagent children can nest at once; raise it +# for wide fan-outs. Invalid or non-positive values fall back to 20. +# HERMES_WEBUI_VISIBLE_SESSION_LIMIT=20 + # Directory scanned for WebUI plugins (default: ~/.hermes/plugins). # HERMES_WEBUI_PLUGINS_DIR=~/.hermes/plugins diff --git a/README.md b/README.md index 7e73b9f1..3edf0f7b 100644 --- a/README.md +++ b/README.md @@ -373,6 +373,7 @@ Full list of environment variables: | `HERMES_HOME` | Windows: `%LOCALAPPDATA%\hermes`; POSIX: `~/.hermes` | Base directory for Hermes state (affects all paths) | | `HERMES_CONFIG_PATH` | `$HERMES_HOME/config.yaml` | Path to Hermes config file | | `HERMES_WEBUI_SERVER_CWD` | *(unset)* | Working directory for the server process. Defaults to the agent dir; point it at a writable workspace when the agent dir is read-only so fallback relative writes land somewhere writable | +| `HERMES_WEBUI_VISIBLE_SESSION_LIMIT` | `20` | Size of the sidebar's interactive recency window (how many recent non-cron/webhook sessions are listed). Also bounds how many delegated subagent children can nest at once, since a child only renders when its row wins a slot in the window — raise it for wide fan-outs. Non-integer or non-positive values fall back to the default | | `HERMES_WEBUI_AGENT_CACHE_MAX` | `25` | Max live agent instances kept warm in the in-memory LRU. Each pins a full conversation transcript, so this is the dominant lever on resident memory — lower it on installs with many long sessions to cap RAM (at the cost of more cold reloads) | | `HERMES_WEBUI_SESSIONS_MAX` | `100` | Legacy operator override for the max compact `Session` objects held in the in-memory LRU. Prefer the `webui.sessions_cache_max` key in `config.yaml` (which takes precedence); this env var remains a fallback. Bounds resident memory so long-running installs cannot accumulate every session ever touched and eventually crash (#4765/#2233/#4633). Eviction only ever drops clean, persisted, non-active sessions; an evicted session lazily reloads from its JSON sidecar on next access | diff --git a/tests/test_visible_session_limit_env.py b/tests/test_visible_session_limit_env.py index 178465f5..e1fc0f32 100644 --- a/tests/test_visible_session_limit_env.py +++ b/tests/test_visible_session_limit_env.py @@ -3,33 +3,69 @@ The sidebar recency window also bounds how many delegated subagent children can render at once, since a child only nests when its row wins a slot in the same payload. Operators running wide fan-outs need to raise it without editing code. + +The constant is bound once at import time, so each case imports ``api.models`` +in a fresh subprocess (same pattern as test_issue3283_profiles_config_import_order) +rather than ``importlib.reload``-ing it in the shared test process — reloading +would recreate the module's locks, caches, and classes while other modules keep +references to the old objects. """ -import importlib +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[1] + +_PROBE = """ +import api.models +print(api.models.CLI_VISIBLE_SESSION_LIMIT) +""" -def _reload_limit(monkeypatch, value): - if value is None: - monkeypatch.delenv("HERMES_WEBUI_VISIBLE_SESSION_LIMIT", raising=False) - else: - monkeypatch.setenv("HERMES_WEBUI_VISIBLE_SESSION_LIMIT", value) - import api.models as models - - return importlib.reload(models).CLI_VISIBLE_SESSION_LIMIT +def _limit_in_fresh_interpreter(tmp_path, value): + home = tmp_path / "home" + home.mkdir() + env = os.environ.copy() + for key in list(env): + if key.startswith("HERMES_WEBUI_") or key in ("HERMES_HOME", "HERMES_BASE_HOME"): + env.pop(key) + env["HOME"] = str(home) + env["PYTHONPATH"] = str(REPO_ROOT) + if value is not None: + env["HERMES_WEBUI_VISIBLE_SESSION_LIMIT"] = value + proc = subprocess.run( + [sys.executable, "-c", _PROBE], + cwd=REPO_ROOT, + env=env, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + timeout=60, + check=False, + ) + assert proc.returncode == 0, proc.stderr + return int(proc.stdout.strip()) -def test_defaults_to_20_when_unset(monkeypatch): - assert _reload_limit(monkeypatch, None) == 20 +def test_defaults_to_20_when_unset(tmp_path): + assert _limit_in_fresh_interpreter(tmp_path, None) == 20 -def test_env_override_raises_the_window(monkeypatch): - assert _reload_limit(monkeypatch, "64") == 64 +def test_env_override_raises_the_window(tmp_path): + assert _limit_in_fresh_interpreter(tmp_path, "64") == 64 -def test_invalid_value_falls_back_to_default(monkeypatch): - assert _reload_limit(monkeypatch, "bogus") == 20 +@pytest.mark.parametrize("value", ["bogus", "0", "-5"]) +def test_invalid_or_nonpositive_falls_back_to_default(tmp_path, value): + assert _limit_in_fresh_interpreter(tmp_path, value) == 20 -def test_zero_and_negative_fall_back_to_default(monkeypatch): - assert _reload_limit(monkeypatch, "0") == 20 - assert _reload_limit(monkeypatch, "-5") == 20 +def test_setting_is_documented(): + readme = (REPO_ROOT / "README.md").read_text(encoding="utf-8") + env_example = (REPO_ROOT / ".env.example").read_text(encoding="utf-8") + assert "| `HERMES_WEBUI_VISIBLE_SESSION_LIMIT` | `20` |" in readme + assert "# HERMES_WEBUI_VISIBLE_SESSION_LIMIT=20" in env_example From c7997164612407b94a3e8918ab279f5a461aa79f Mon Sep 17 00:00:00 2001 From: happy5318 Date: Mon, 14 Sep 2026 22:52:32 +0800 Subject: [PATCH 22/57] perf(run-journal): rebuild live snapshots without quadratic work `_run_journal_live_snapshot` rebuilds the live UI state for an active run by replaying the whole durable journal. On a long run (22.7k events / 15MB) that rebuild took ~4.4s per session load and blocked the HTTP thread - the dominant cost of "switching to a session with a task running takes forever". Four output-preserving fixes: 1. Locate the journal without parsing it (`find_run_file`) and derive the durable summary from the single `read_run_events` parse instead of parsing the file twice (`find_run_summary` + `read_run_events`). The historical `find_run_summary` lookup is kept as the fallback when no journal file exists on disk. 2. Skip `metering` rows in the projection loop (~60% of a long live journal's bytes; nothing in the snapshot is built from them), while preserving the last_ts watermark they carry. 3. Collect reasoning deltas into a list and materialize lazily instead of `reasoning_text += chunk` (quadratic inside these closures: ~2.3s of the replay on a 9.7k-chunk run). 4. Fold only a bounded tail of the reasoning transcript for the interim-echo suffix check instead of `re.sub` over the whole growing text (~1.5s over 100+ probes). Measured on a 15MB / 22.7k-row live journal: 4.34s -> 0.49s (8.9x) with byte-identical snapshot output (sha256 of the canonical JSON unchanged). Related work: #7310 (slow /api/session under load), #7534 (incremental journal read cache - complementary: this removes one of the two parses entirely and fixes the replay-loop hotspots), #7273/#7291 (metering write-side skip - the read-side loop skip here complements them for legacy journals that still carry metering rows). --- api/routes.py | 82 +++++++++++++++++++++++++++++++++++++++------- api/run_journal.py | 21 ++++++++++++ 2 files changed, 91 insertions(+), 12 deletions(-) diff --git a/api/routes.py b/api/routes.py index 7bfa4347..cf1c86d7 100644 --- a/api/routes.py +++ b/api/routes.py @@ -3461,16 +3461,31 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | emit_error=False, ): return None - summary = find_run_summary(stream_id) - if not summary: - return None - session_id = str(summary.get("session_id") or "") - if not session_id: - return None + # Locate the run journal WITHOUT parsing it first. find_run_summary reads + # and parses the whole file (tens of thousands of rows on a long live + # run), and read_run_events below then parsed it AGAIN — two full passes + # cost ~0.6s per rebuild. Derive the durable summary from the single + # parse below instead; its last_seq / last_event_id are rebuilt from the + # same rows the snapshot consumes. + located = find_run_file(stream_id) + if located: + session_id, _journal_path = located + else: + # No journal file on disk for this run id: fall back to the + # historical summary-based lookup seam (defensive — a real miss + # returns None exactly like before; this is also the seam that + # long-standing tests stub with a handler-side summary). + fallback_summary = find_run_summary(stream_id) + if not fallback_summary: + return None + session_id = str(fallback_summary.get("session_id") or "") + if not session_id: + return None journal = read_run_events(session_id, stream_id) events = [event for event in (journal.get("events") or []) if isinstance(event, dict)] if not events: return None + summary = _summary_from_events(session_id, stream_id, events) event_run_ids: set[str] = set() malformed_envelope_run_id = False for event in events: @@ -3490,6 +3505,14 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | assistant_text = "" reasoning_text = "" + # Reasoning deltas arrive as thousands of tiny append events. Growing the + # transcript with ``reasoning_text += chunk`` copies the full (multi- + # hundred-KB) string on every append inside these closures — quadratic + # (~2.3s of a 4.6s rebuild on a 9.7k-chunk run). Collect chunks and + # materialize lazily; readers (echo probes, strip, final message) join at + # most once per interim segment. + reasoning_parts: list[str] = [] + reasoning_dirty = False messages: list[dict] = [] tool_calls: list[dict] = [] activity_burst_anchors: list[dict] = [] @@ -3498,6 +3521,13 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | last_ts = None reasoning_first_tool_count: int | None = None + def _materialize_reasoning_text() -> str: + nonlocal reasoning_text, reasoning_dirty + if reasoning_dirty: + reasoning_text = "".join(reasoning_parts) + reasoning_dirty = False + return reasoning_text + def mark_boundary() -> int: nonlocal current_activity_burst_id text_end = len(assistant_text) @@ -3565,19 +3595,41 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | candidate = _compact_for_echo_compare(text) if not candidate: return False - return _compact_for_echo_compare(reasoning_text).endswith(candidate) + full = _materialize_reasoning_text() + if not full: + return False + # Suffix test — folding the WHOLE (growing) reasoning transcript per + # interim segment ran re.sub over hundreds of KB 100+ times per + # rebuild (~1.5s). Fold only a bounded tail: compact() only drops + # whitespace, so a window of len(text)*3 characters covers the echo's + # non-whitespace characters — the same window contract + # _strip_compact_echo_suffix already uses for the reverse operation. + tail = full[-max(len(str(text or "")) * 3, 4096):] + return _compact_for_echo_compare(tail).endswith(candidate) def strip_reasoning_echo_tail(text: str) -> bool: - nonlocal reasoning_text, reasoning_first_tool_count - next_reasoning, did_remove = _strip_compact_echo_suffix(reasoning_text, text) + nonlocal reasoning_text, reasoning_dirty, reasoning_first_tool_count + next_reasoning, did_remove = _strip_compact_echo_suffix( + _materialize_reasoning_text(), text + ) if did_remove: reasoning_text = next_reasoning + reasoning_parts.clear() + reasoning_parts.append(next_reasoning) + reasoning_dirty = False if not _compact_for_echo_compare(reasoning_text): reasoning_first_tool_count = None return did_remove for event in events: event_name = str(event.get("event") or event.get("type") or "") + if event_name == "metering": + # Metering rows are the bulk of a long live journal (~10k rows, + # ~60% of its bytes) and project nothing onto the snapshot; skip + # their per-row work while preserving the last_ts watermark they + # carry. + last_ts = event.get("created_at", last_ts) + continue payload = event.get("payload") if isinstance(event.get("payload"), dict) else {} last_ts = event.get("created_at", last_ts) if event_name == "token": @@ -3588,9 +3640,11 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | continue if event_name == "reasoning": text = str(payload.get("text") or "") - if text and reasoning_first_tool_count is None: - reasoning_first_tool_count = len(tool_calls) - reasoning_text += text + if text: + if reasoning_first_tool_count is None: + reasoning_first_tool_count = len(tool_calls) + reasoning_parts.append(text) + reasoning_dirty = True continue if event_name == "interim_assistant": visible = str(payload.get("text") or "").strip() @@ -3635,6 +3689,8 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | update_completed_tool(payload) fresh_segment = True + reasoning_text = _materialize_reasoning_text() + if assistant_text or reasoning_text: message = { "role": "assistant", @@ -10708,7 +10764,9 @@ from api.streaming import ( from api.gateway_chat import _run_gateway_chat_streaming, webui_gateway_chat_enabled from api.run_journal import ( _parse_run_journal_event_id as _shared_parse_run_journal_event_id, + _summary_from_events, bound_run_journal_snapshot_args, + find_run_file, find_run_summary, read_run_events, read_session_run_events, diff --git a/api/run_journal.py b/api/run_journal.py index ab8d0a7e..0031b1a6 100644 --- a/api/run_journal.py +++ b/api/run_journal.py @@ -587,6 +587,27 @@ def find_run_summary(run_id: str, *, session_dir: Path | None = None) -> dict | return None +def find_run_file(run_id: str, *, session_dir: Path | None = None) -> tuple[str, Path] | None: + """Locate a run journal file by run id WITHOUT parsing its body. + + Hot callers that immediately read the full journal (the live-snapshot + rebuild) must not pay :func:`find_run_summary`'s full-file parse first: + on a long live run the file holds tens of thousands of rows and parsing + it twice per rebuild dominated the snapshot cost. Returns + ``(session_id, path)`` for the first match, or ``None`` when the run id + is invalid or no journal exists. + """ + try: + rid = _validate_id(run_id, "run_id") + except ValueError: + return None + root = Path(session_dir) if session_dir is not None else _default_session_dir() + journal_root = root / RUN_JOURNAL_DIR_NAME + for path in journal_root.glob(f"*/{rid}.jsonl"): + return path.parent.name, path + return None + + def read_session_run_events( session_id: str, *, From df7db1d3e2cbcc04501b3b264ef33c6e57087363 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Mon, 14 Sep 2026 22:54:49 +0800 Subject: [PATCH 23/57] test(run-journal): pin single-parse locate + summary fallback seam --- tests/test_live_snapshot_single_parse.py | 114 +++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 tests/test_live_snapshot_single_parse.py diff --git a/tests/test_live_snapshot_single_parse.py b/tests/test_live_snapshot_single_parse.py new file mode 100644 index 00000000..7d88b08c --- /dev/null +++ b/tests/test_live_snapshot_single_parse.py @@ -0,0 +1,114 @@ +"""Regression: live-snapshot rebuild locates the journal with a single parse. + +``_run_journal_live_snapshot`` used ``find_run_summary`` (full parse) and +then ``read_run_events`` (second full parse) - two passes over the whole +journal per rebuild (~0.6s+ of a 4.4s rebuild on a 22.7k-row live run). +``find_run_file`` now locates the file without parsing and the durable +summary is derived from the single events parse; the historical +``find_run_summary`` lookup remains as the fallback seam when no journal +file exists on disk. +""" + +import json + +import api.routes as routes +from api import run_journal as RJ + + +def _write_journal(root, session_id, run_id, count=5): + session_root = root / RJ.RUN_JOURNAL_DIR_NAME / session_id + session_root.mkdir(parents=True, exist_ok=True) + path = session_root / f"{run_id}.jsonl" + rows = [] + for seq in range(1, count + 1): + rows.append( + json.dumps( + { + "version": 1, + "event_id": f"{run_id}:{seq}", + "seq": seq, + "run_id": run_id, + "session_id": session_id, + "event": "token", + "type": "token", + "created_at": 1000 + seq, + "payload": {"text": "x"}, + } + ) + ) + path.write_text("\n".join(rows) + "\n", encoding="utf-8") + return path + + +def test_find_run_file_locates_without_parsing(tmp_path): + path = _write_journal(tmp_path, "session_1", "run_1") + located = RJ.find_run_file("run_1", session_dir=tmp_path) + assert located is not None + session_id, located_path = located + assert session_id == "session_1" + assert located_path == path + assert RJ.find_run_file("missing_run", session_dir=tmp_path) is None + assert RJ.find_run_file("../evil", session_dir=tmp_path) is None + + +def _forbid_summary_parse(*_a, **_k): + raise AssertionError( + "find_run_summary must not be parsed when the journal file exists on " + "disk - the single-parse locate path regressed" + ) + + +def test_live_snapshot_rebuilds_from_single_parse(tmp_path, monkeypatch): + _write_journal(tmp_path, "session_1", "run_1", count=3) + monkeypatch.setattr(RJ, "_default_session_dir", lambda: tmp_path) + # The double-parse fix: when a journal file exists, the rebuild must NOT + # route through find_run_summary (that would parse the file a second time). + monkeypatch.setattr(routes, "find_run_summary", _forbid_summary_parse) + + snapshot = routes._run_journal_live_snapshot("run_1") + + assert snapshot is not None + assert snapshot["session_id"] == "session_1" + assert snapshot["stream_id"] == "run_1" + assert snapshot["last_seq"] == 3 + assert snapshot["event_count"] == 3 + assert snapshot["last_assistant_text"] == "xxx" + + +def test_live_snapshot_summary_fallback_seam(tmp_path, monkeypatch): + """No journal file on disk -> the historical find_run_summary seam is used.""" + monkeypatch.setattr(RJ, "_default_session_dir", lambda: tmp_path) + monkeypatch.setattr( + routes, + "find_run_summary", + lambda stream_id: { + "session_id": "session_1", + "run_id": stream_id, + "last_seq": 1, + "last_event_id": f"{stream_id}:1", + }, + ) + monkeypatch.setattr( + routes, + "read_run_events", + lambda sid, rid: { + "events": [ + { + "version": 1, + "event_id": f"{rid}:1", + "seq": 1, + "run_id": rid, + "session_id": sid, + "event": "token", + "type": "token", + "created_at": 1000, + "payload": {"text": "hello"}, + } + ], + }, + ) + + snapshot = routes._run_journal_live_snapshot("run_1") + + assert snapshot is not None + assert snapshot["last_assistant_text"] == "hello" \ No newline at end of file From 3369bc6a6d9260300fec3c99195830afc4131c22 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Tue, 15 Sep 2026 08:00:51 +0800 Subject: [PATCH 24/57] fix(run-journal): match echo suffixes with a window-free backward walk Review follow-up: the bounded echo probe folded at most len(text)*3 raw characters, so a compact-equivalent suffix whose raw span was stretched by interior whitespace was missed - the echo strip was dropped and the interim text was duplicated in the restored transcript. The same retired window existed in the reverse helper (_strip_compact_echo_suffix), pinned by its own test. Both now share _find_compact_echo_suffix_start: a backward walk over value/suffix that skips whitespace in value, inspects only the echo span, and allocates nothing. It restores the previous output semantics (origin/master folded the whole reasoning text) and is cheaper than the windowed version (no per-call tail fold). Tests: new journal-reconstruction regression (a 5000-space-stretched echo must appear exactly once; genuine interim text must not be treated as an echo), the suffix-helper oracle updated to window-free semantics, and the window-contract test now pins that a stretched echo IS stripped. --- api/routes.py | 19 ++-- api/streaming.py | 73 +++++++----- tests/test_compact_echo_suffix_linear_scan.py | 70 +++++------- tests/test_live_snapshot_echo_dedup.py | 104 ++++++++++++++++++ 4 files changed, 186 insertions(+), 80 deletions(-) create mode 100644 tests/test_live_snapshot_echo_dedup.py diff --git a/api/routes.py b/api/routes.py index cf1c86d7..de75c1fd 100644 --- a/api/routes.py +++ b/api/routes.py @@ -3592,20 +3592,16 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | tool_calls.append(call) def reasoning_echo_tail_matches(text: str) -> bool: - candidate = _compact_for_echo_compare(text) - if not candidate: - return False full = _materialize_reasoning_text() if not full: return False - # Suffix test — folding the WHOLE (growing) reasoning transcript per - # interim segment ran re.sub over hundreds of KB 100+ times per - # rebuild (~1.5s). Fold only a bounded tail: compact() only drops - # whitespace, so a window of len(text)*3 characters covers the echo's - # non-whitespace characters — the same window contract - # _strip_compact_echo_suffix already uses for the reverse operation. - tail = full[-max(len(str(text or "")) * 3, 4096):] - return _compact_for_echo_compare(tail).endswith(candidate) + # Backward compact-suffix walk (no fixed window): linear in the echo + # length, allocation-free, and it cannot miss a compact-equivalent + # suffix whose raw span is stretched by interior whitespace. The + # previous bounded-tail probe folded up to len(text)*3 raw characters + # per interim segment - and when its window fell short it dropped the + # echo strip, duplicating the interim text in the restored transcript. + return _find_compact_echo_suffix_start(full, text) is not None def strip_reasoning_echo_tail(text: str) -> bool: nonlocal reasoning_text, reasoning_dirty, reasoning_first_tool_count @@ -10759,6 +10755,7 @@ from api.streaming import ( _materialize_pending_user_turn_before_error, generate_session_title_for_session, _compact_for_echo_compare, + _find_compact_echo_suffix_start, _strip_compact_echo_suffix, ) from api.gateway_chat import _run_gateway_chat_streaming, webui_gateway_chat_enabled diff --git a/api/streaming.py b/api/streaming.py index 29cda8be..4c86104e 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -197,41 +197,58 @@ def _compact_for_echo_compare(value: str) -> str: return re.sub(r'\s+', '', str(value or '')) -def _strip_compact_echo_suffix(value: str, suffix: str, *, search_window: int = 4096) -> tuple[str, bool]: +def _find_compact_echo_suffix_start(value: str, suffix: str) -> int | None: + """Return the index where a whitespace-folded ``suffix`` starts at the + end of ``value``, or ``None`` when the tail does not echo it. + + The match walks ``value`` and ``suffix`` from the end, skipping + whitespace in ``value``; only the echo span itself is inspected, so the + cost is linear in the echo length and allocation-free. Unlike a fixed + fold window, the walk cannot miss a compact-equivalent suffix whose raw + span is stretched by interior whitespace. + + ``str.isspace`` is used for the walk instead of the ``\\s`` pattern used + by :func:`_compact_for_echo_compare`. The two agree on every Unicode code + point, so the folded view and the walk stay consistent. + """ + candidate = _compact_for_echo_compare(suffix) + if not candidate: + return None + i = len(value) - 1 + j = len(candidate) - 1 + while j >= 0: + while i >= 0 and value[i].isspace(): + i -= 1 + if i < 0 or value[i] != candidate[j]: + return None + i -= 1 + j -= 1 + return i + 1 + + +def _strip_compact_echo_suffix(value: str, suffix: str) -> tuple[str, bool]: """Remove ``suffix`` from ``value`` when they match after whitespace folding. - The search window is folded once and the cut point is then located by - walking backwards across the echo itself. The previous implementation - probed every candidate cut index and re-folded the whole remaining tail for - each probe, which is quadratic in the window size: a 6000-character final - message cost seconds of CPU, held under the GIL, stalling every other - stream in the process. + The cut point is located by the same backward walk as + :func:`_find_compact_echo_suffix_start`: no fixed search window is + involved, so a compact-equivalent suffix is removed no matter how much + interior whitespace stretches its raw span. The previous windowed + implementation folded a bounded tail on every call; its retired probing + variant re-folded the remaining tail per candidate cut index, which is + quadratic: a 6000-character final message cost seconds of CPU, held under + the GIL, stalling every other stream in the process. - ``str.isspace`` is used for the backwards walk instead of the ``\\s`` - pattern used by :func:`_compact_for_echo_compare`. The two agree on every - Unicode code point, so the folded view and the walk stay consistent. + Whitespace sitting between the kept text and the echo is removed by + ``rstrip``, which lands on the same result as the leftmost cut index the + probing loop used to return. """ raw = str(value or '') - candidate = _compact_for_echo_compare(suffix) - if not raw or not candidate: + if not raw: return raw, False - tail = raw[-max(len(str(suffix or '')) * 3, search_window):] - offset = len(raw) - len(tail) - compact_tail = _compact_for_echo_compare(tail) - if len(candidate) > len(compact_tail) or not compact_tail.endswith(candidate): + start = _find_compact_echo_suffix_start(raw, suffix) + if start is None: return raw, False - # Consume exactly as many non-whitespace characters as the folded suffix - # holds; ``idx`` then sits on the first character of the echo. Whitespace - # sitting between the kept text and the echo is removed by ``rstrip``, - # which is why this lands on the same result as the leftmost cut index the - # probing loop used to return. - remaining = len(candidate) - idx = len(tail) - while remaining and idx: - idx -= 1 - if not tail[idx].isspace(): - remaining -= 1 - return raw[: offset + idx].rstrip(), True + return raw[:start].rstrip(), True def _redacted_session_payload_with_full_messages(session, *, tool_calls=None) -> dict | None: diff --git a/tests/test_compact_echo_suffix_linear_scan.py b/tests/test_compact_echo_suffix_linear_scan.py index 21c66aec..1a0b0c00 100644 --- a/tests/test_compact_echo_suffix_linear_scan.py +++ b/tests/test_compact_echo_suffix_linear_scan.py @@ -7,11 +7,16 @@ with ``window x suffix`` instead of with the data actually inspected. On a 6000-character conclusion block that is seconds of pure CPU, held under the GIL, which stalls every other stream in the process. +The current implementation walks ``value`` and ``suffix`` backwards from the +end (skipping whitespace in ``value``) and has no fixed search window, so a +compact-equivalent echo is found regardless of how much interior whitespace +stretches its raw span. + These tests pin both halves of the contract: * the fast path must return byte-identical results to a straightforward reference implementation, including the leftmost-cut tie-break; and -* it must not re-fold the search window once per candidate cut index. +* it must not fold the whole buffer once per candidate cut index. """ import random import re @@ -20,11 +25,12 @@ import string import pytest -def _reference_strip_compact_echo_suffix(value, suffix, *, search_window: int = 4096): - """Deliberately naive oracle: probe every cut index, fold the whole tail. +def _reference_strip_compact_echo_suffix(value, suffix): + """Deliberately naive oracle: probe every cut index, fold the tail. - This mirrors the original behaviour exactly and exists only so the - optimised implementation can be proven equivalent to it. + This mirrors the pre-window semantics (the original probing loop) with no + search window, and exists only so the optimised backward walk can be + proven equivalent to it. """ def compact(v): return re.sub(r'\s+', '', str(v or '')) @@ -33,11 +39,9 @@ def _reference_strip_compact_echo_suffix(value, suffix, *, search_window: int = candidate = compact(suffix) if not raw or not candidate: return raw, False - tail = raw[-max(len(str(suffix or '')) * 3, search_window):] - offset = len(raw) - len(tail) - for idx in range(len(tail) + 1): - if compact(tail[idx:]) == candidate: - return raw[: offset + idx].rstrip(), True + for idx in range(len(raw) + 1): + if compact(raw[idx:]) == candidate: + return raw[:idx].rstrip(), True return raw, False @@ -74,8 +78,8 @@ def _cases(): ("aaaa", "aa"), # Long buffer with the echo at the very end. ("remplissage " * 500 + "la vraie conclusion", "la vraie conclusion"), - # Echo whose whitespace-padded span is wider than the search window: - # the window only exposes ``conclusion``, so nothing may be stripped. + # Echo whose whitespace-padded span is wider than any fixed fold + # window: the backward walk must still strip it. ("bla une" + " " * 5000 + "conclusion", "une conclusion"), # Exotic whitespace that ``\s`` and ``str.isspace`` must treat alike. ("bla\u00a0bla une conclusion", "une\u00a0conclusion"), @@ -111,45 +115,29 @@ def test_strip_compact_echo_suffix_matches_reference_oracle(): ) -def test_strip_compact_echo_suffix_honours_a_custom_search_window(): - """The bounded window must keep its meaning on the fast path.""" - import api.streaming as streaming - - buffer_text = "z" * 400 + " la conclusion" - for window in (16, 64, 512, 4096): - assert streaming._strip_compact_echo_suffix( - buffer_text, "la conclusion", search_window=window - ) == _reference_strip_compact_echo_suffix( - buffer_text, "la conclusion", search_window=window - ), f"window={window}" - - -def test_strip_compact_echo_suffix_rejects_an_echo_wider_than_the_window(): - """An echo that only fits inside a wider window must not be found. +def test_strip_compact_echo_suffix_finds_an_echo_wider_than_any_fixed_window(): + """An echo stretched past any fold window must still be stripped. The buffer ends with the echo, but interior whitespace stretches its raw - span past the default window, so the folded window only exposes the last - word. Widening the window to cover the whole span makes the same echo - visible again, which proves the non-match is caused by the window and - not by the text. + span far beyond a fixed fold window (the retired windowed implementation + exposed only the last word and left the buffer untouched). The backward + walk has no window, so the cut point is found and the echo is removed. """ import api.streaming as streaming buffer_text = "bla une" + " " * 5000 + "conclusion" suffix = "une conclusion" - for impl in (streaming._strip_compact_echo_suffix, _reference_strip_compact_echo_suffix): - assert impl(buffer_text, suffix) == (buffer_text, False), impl.__name__ - assert impl(buffer_text, suffix, search_window=len(buffer_text)) == ("bla", True), impl.__name__ + assert streaming._strip_compact_echo_suffix(buffer_text, suffix) == ("bla", True) + assert _reference_strip_compact_echo_suffix(buffer_text, suffix) == ("bla", True) -def test_strip_compact_echo_suffix_does_not_refold_the_window_per_cut(monkeypatch): +def test_strip_compact_echo_suffix_does_not_refold_per_cut(monkeypatch): """One pass, not one fold per candidate cut index. - The original implementation called the whitespace-folding helper once per - possible cut position — roughly 4097 times for a default window — and each - of those calls re-scanned the remaining tail. Counting the calls pins the - algorithmic property directly, without depending on wall-clock timing. + Counting the folding-helper calls pins the algorithmic property directly, + without depending on wall-clock timing: the backward walk folds the + suffix once and then compares characters. """ import api.streaming as streaming @@ -168,7 +156,7 @@ def test_strip_compact_echo_suffix_does_not_refold_the_window_per_cut(monkeypatc assert calls['n'] <= 8, ( f"whitespace folding ran {calls['n']} times for a single call; the scan " - "is re-folding the window once per candidate cut index" + "is re-folding once per candidate cut index" ) @@ -193,4 +181,4 @@ def test_strip_compact_echo_suffix_stays_cheap_on_a_long_conclusion(): # The original implementation needs ~7s here. A very loose budget keeps the # test meaningful on a loaded CI box while still failing the quadratic scan # by three orders of magnitude. - assert elapsed < 0.5, f"single call took {elapsed:.3f}s" + assert elapsed < 0.5, f"single call took {elapsed:.3f}s" \ No newline at end of file diff --git a/tests/test_live_snapshot_echo_dedup.py b/tests/test_live_snapshot_echo_dedup.py new file mode 100644 index 00000000..6bd55cb3 --- /dev/null +++ b/tests/test_live_snapshot_echo_dedup.py @@ -0,0 +1,104 @@ +"""Journal-reconstruction regression: a whitespace-stretched reasoning echo +must be de-duplicated exactly once. + +``_run_journal_live_snapshot`` strips reasoning echoes of interim assistant +text so the restored transcript does not show the same content in both the +thinking block and the assistant output (run-state consistency invariant: +replay must not duplicate interim assistant text). The previous bounded echo +probe could miss a compact-equivalent suffix whose raw span exceeded its fold +window, leaving the duplicate in the snapshot; the backward walk has no +window. + +Both directions are pinned: a stretched echo is stripped (appears once), and +genuine interim text that is NOT a reasoning echo is left alone. +""" + +import json +import re + +import api.routes as routes +from api import run_journal as RJ + + +def _compact(value): + return re.sub(r"\s+", "", str(value or "")) + + +def _write_journal(root, session_id, run_id, events): + session_root = root / RJ.RUN_JOURNAL_DIR_NAME / session_id + session_root.mkdir(parents=True, exist_ok=True) + path = session_root / f"{run_id}.jsonl" + rows = [] + for seq, (event, payload) in enumerate(events, start=1): + rows.append( + json.dumps( + { + "version": 1, + "event_id": f"{run_id}:{seq}", + "seq": seq, + "run_id": run_id, + "session_id": session_id, + "event": event, + "type": event, + "created_at": 1000 + seq, + "payload": payload, + } + ) + ) + path.write_text("\n".join(rows) + "\n", encoding="utf-8") + return path + + +def test_whitespace_stretched_reasoning_echo_appears_once(tmp_path, monkeypatch): + gap = " " * 5000 # raw echo span far beyond any fixed fold window + _write_journal( + tmp_path, + "session_1", + "run_1", + [ + ("reasoning", {"text": "prefix " + "alpha" + gap + "beta"}), + ("interim_assistant", {"text": "alpha beta"}), + ], + ) + monkeypatch.setattr(RJ, "_default_session_dir", lambda: tmp_path) + + snapshot = routes._run_journal_live_snapshot("run_1") + + message = snapshot["messages"][-1] + reasoning_text = message.get("reasoning") or "" + content_text = message.get("content") or "" + occurrences = _compact(reasoning_text).count("alphabeta") + _compact( + content_text + ).count("alphabeta") + assert occurrences == 1, ( + f"the interim echo must appear once after reconstruction; " + f"reasoning={reasoning_text[-60:]!r} content={content_text[-60:]!r}" + ) + # The echo span is stripped from the reasoning copy; the prefix stays. + assert "alphabeta" not in _compact(reasoning_text) + assert "alpha beta" in content_text + + +def test_genuine_interim_text_is_not_treated_as_echo(tmp_path, monkeypatch): + gap = " " * 5000 + _write_journal( + tmp_path, + "session_1", + "run_1", + [ + ("reasoning", {"text": "prefix " + "alpha" + gap + "beta"}), + ("interim_assistant", {"text": "gamma delta"}), + ], + ) + monkeypatch.setattr(RJ, "_default_session_dir", lambda: tmp_path) + + snapshot = routes._run_journal_live_snapshot("run_1") + + message = snapshot["messages"][-1] + reasoning_text = message.get("reasoning") or "" + content_text = message.get("content") or "" + # The genuine interim answer is preserved in full... + assert "gamma delta" in content_text + # ...and the reasoning transcript is not suppressed by the matcher. + assert _compact(reasoning_text).count("alphabeta") == 1 + assert "prefix" in reasoning_text From 3557982881aa861921e6dd40abddf1badd7b1849 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Tue, 15 Sep 2026 08:01:54 +0800 Subject: [PATCH 25/57] docs(run-state-rfc): note the window-free echo de-duplication semantics --- docs/rfcs/webui-run-state-consistency-contract.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index 9b6d02ec..1954abca 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -131,6 +131,18 @@ and 5; it does not mark every run-state boundary implemented. Visible interim assistant progress must remain visible timeline content; a compact Activity disclosure may summarize adjacent tool/debug detail, but it must not be the only place where the user can see emitted progress text. + Interim assistant text that duplicates the tail of the accumulated reasoning + transcript is stripped from the reasoning copy so the restored snapshot + shows the content once. That echo match is whitespace-insensitive and + carries no fixed search window: a compact-equivalent suffix is recognized + however much interior whitespace stretches its raw span + (`_find_compact_echo_suffix_start` / `_strip_compact_echo_suffix` in + `api/streaming.py`, shared with the live-stream echo path; the live journal + replay in `api/routes.py` uses the same matcher). The retired windowed + variants could drop the strip when the span exceeded the window, duplicating + the interim text; the journal-reconstruction regression + (`tests/test_live_snapshot_echo_dedup.py`) pins the single-occurrence + result. 6. **Compression is not current intent.** Automatic compression summaries and reference cards are recovery/handoff material. They must not be treated as a new user request, active-turn content, or the default visible explanation for From dcac71c74e0c3bddf6e407efdb98f7ee53f8007e Mon Sep 17 00:00:00 2001 From: happy5318 <5318happy@users.noreply.github.com> Date: Sun, 20 Sep 2026 20:01:55 +0800 Subject: [PATCH 26/57] fix(run-journal): match echo suffixes through an incremental folded index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up for #7569: change 4 of the live-snapshot perf PR replaced one quadratic with another. The window-free backward walk is correct, but it re-walks the raw reasoning transcript's whitespace once per interim event — on a whitespace-heavy transcript that span is megabytes, and the replay went ~55x slower than master on the production shape the gate measured (1MB / 20-interim journal: 0.035s master → 1.922s branch). ## Thinking Path The gate's finding was that the cost is structural, not a tweak: the matcher consults the *whole* accumulated transcript on every interim event, so any raw-text rescan of it is O(interim_count x whitespace_span). The fix is the data-structure change the review asked for — maintain the folded view and the raw cut offsets incrementally, and match/strip from that indexed state instead of rescanning the raw buffer. ## What Changed - `api/streaming.py`: new `_CompactEchoIndex` — an append-only folded index that records, per non-whitespace character, its raw index. `append` folds a chunk once (cost proportional to the chunk, never the buffer), `matches_tail`/`cut_to` compare only the candidate against the folded tail (O(len(candidate)), no raw walk), `reset` drops the state after the raw text is truncated. The old raw helpers (`_find_compact_echo_suffix_start`, `_strip_compact_echo_suffix`) stay as the equivalence oracle. - `api/routes.py::_run_journal_live_snapshot`: reasoning chunks feed the index as they are appended; `reasoning_echo_tail_matches` and `strip_reasoning_echo_tail` consult the index. The strip re-indexes the truncated transcript so later probes match against it. - `api/streaming.py` live path: the same swap for the live-stream echo strip. The three live reasoning buffers (STREAM_REASONING_TEXT, the coalescing buffer, the per-message segments) each carry a mirrored index, fed on every append and re-indexed after a strip. The buffer flush resets the buffer's index with the text it drops, so the index never describes text that is no longer there. - `docs/rfcs/webui-run-state-consistency-contract.md`: the echo-match clause now names the indexed representation and the scaling regression that pins it. ## Why It Matters Removing one quadratic by introducing another in the same hot path is the exact risk the gate flagged — the headline benchmark used a journal shape where the window-free walk's cost does not dominate, so the aggregate number still looked like a win. The index keeps the original 8.9x and adds the scaling guarantee on the shape that was regressing. ## Verification - Measured on the reproduced shape (1MB whitespace span, 20 interim events): 0.972s → 0.049s. Doubling both span and interim count scales ~2x per step (x1 0.041s / x2 0.090s / x4 0.175s) — linear, not quadratic. - Behaviour equivalence: 300 randomized journals (whitespace-heavy, Unicode, chunked, flagged/unflagged interims) rebuilt with the indexed implementation and with the raw walk swapped in — sha256 of the canonical snapshot JSON matched on all 300. - `tests/test_compact_echo_index.py` (new, 6 tests): oracle equivalence over 2000+ randomized cases, chunked-append equivalence, matches_tail/cut_to agreement, reset-after-truncation, and two cost probes that fail the raw walk. - `tests/test_live_snapshot_echo_scan_scaling.py` (new, 4 tests): the proportional-scaling regression the review asked for — verified to FAIL on the pre-fix matcher (14.6x time for 4x rows, the quadratic signature) and pass on the indexed one. - Existing echo/journal suites unchanged and green: 19 items across `test_compact_echo_suffix_linear_scan` / `test_live_snapshot_echo_dedup` / `test_live_snapshot_single_parse`, plus 66 neighbouring items (`test_issue1361_cancel_data_loss`, `test_issue2565_reasoning_accumulation`, `test_issue2713_streaming_segment_flush`, `test_cancel_interrupt`, `test_run_journal`). ## Contract Change None. The matcher's observable behaviour is unchanged (same cut points, same whitespace-insensitive semantics); only its cost profile changes. No config knobs, no API surface. ## Risks - Index drift: if a future writer mutates one of the reasoning buffers without feeding (or resetting) its index, the matcher would silently miss echoes. Mitigated by keeping the index feed adjacent to every existing write site and by the reset-on-flush/reset-on-strip symmetry; the equivalence test would catch a dropped feed as a missed echo. - Memory: the index stores one char plus one int per non-whitespace character. On a whitespace-heavy transcript this is bounded by the folded length, which is what the transcript would cost folded anyway; the raw text is still the larger object. ## Model Used Hermes Agent (step-5-preview) with direct file edits and pytest verification. Cost properties were measured with a purpose-built scaling probe (proportional span x interim count), not a fixed-size fixture; the probe is kept as a regression test rather than a script. ## Related #7569 (this PR), #7310 (slow /api/session under load), #7534 (incremental journal read cache — complementary), #7273/#7291 (metering write-side skip). --- api/routes.py | 50 ++-- api/streaming.py | 151 +++++++++++- .../webui-run-state-consistency-contract.md | 21 +- tests/test_compact_echo_index.py | 218 ++++++++++++++++++ tests/test_live_snapshot_echo_scan_scaling.py | 127 ++++++++++ 5 files changed, 526 insertions(+), 41 deletions(-) create mode 100644 tests/test_compact_echo_index.py create mode 100644 tests/test_live_snapshot_echo_scan_scaling.py diff --git a/api/routes.py b/api/routes.py index de75c1fd..db182efb 100644 --- a/api/routes.py +++ b/api/routes.py @@ -3513,6 +3513,10 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | # most once per interim segment. reasoning_parts: list[str] = [] reasoning_dirty = False + # Incremental folded index over the reasoning transcript: the per-interim + # echo probe consults this instead of re-walking the raw text (see + # ``_CompactEchoIndex``). + reasoning_index = _CompactEchoIndex() messages: list[dict] = [] tool_calls: list[dict] = [] activity_burst_anchors: list[dict] = [] @@ -3592,30 +3596,32 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | tool_calls.append(call) def reasoning_echo_tail_matches(text: str) -> bool: - full = _materialize_reasoning_text() - if not full: - return False - # Backward compact-suffix walk (no fixed window): linear in the echo - # length, allocation-free, and it cannot miss a compact-equivalent - # suffix whose raw span is stretched by interior whitespace. The - # previous bounded-tail probe folded up to len(text)*3 raw characters - # per interim segment - and when its window fell short it dropped the - # echo strip, duplicating the interim text in the restored transcript. - return _find_compact_echo_suffix_start(full, text) is not None + # Indexed tail match (no raw-text walk): the incremental index folds + # each reasoning chunk once as it is appended, so this probe costs + # O(len(text)) regardless of how much interior whitespace stretches + # the raw span. The previous raw backward walk re-walked that span + # once per interim event, which turned the replay quadratic on a + # whitespace-heavy transcript (#7569 review, ~55x slower than master + # on a production-shaped journal). + return reasoning_index.matches_tail(text) def strip_reasoning_echo_tail(text: str) -> bool: nonlocal reasoning_text, reasoning_dirty, reasoning_first_tool_count - next_reasoning, did_remove = _strip_compact_echo_suffix( - _materialize_reasoning_text(), text - ) - if did_remove: - reasoning_text = next_reasoning - reasoning_parts.clear() - reasoning_parts.append(next_reasoning) - reasoning_dirty = False - if not _compact_for_echo_compare(reasoning_text): - reasoning_first_tool_count = None - return did_remove + cut = reasoning_index.cut_to(text) + if cut is None: + return False + next_reasoning = _materialize_reasoning_text()[:cut].rstrip() + reasoning_text = next_reasoning + reasoning_parts.clear() + reasoning_parts.append(next_reasoning) + reasoning_dirty = False + # Re-index the truncated transcript so later probes match against it + # instead of the pre-strip tail (the index is the match authority). + reasoning_index.reset() + reasoning_index.append(next_reasoning) + if not _compact_for_echo_compare(reasoning_text): + reasoning_first_tool_count = None + return True for event in events: event_name = str(event.get("event") or event.get("type") or "") @@ -3640,6 +3646,7 @@ def _run_journal_live_snapshot(stream_id: str | None, *, handler=None) -> dict | if reasoning_first_tool_count is None: reasoning_first_tool_count = len(tool_calls) reasoning_parts.append(text) + reasoning_index.append(text) reasoning_dirty = True continue if event_name == "interim_assistant": @@ -10755,6 +10762,7 @@ from api.streaming import ( _materialize_pending_user_turn_before_error, generate_session_title_for_session, _compact_for_echo_compare, + _CompactEchoIndex, _find_compact_echo_suffix_start, _strip_compact_echo_suffix, ) diff --git a/api/streaming.py b/api/streaming.py index 4c86104e..b55cfb9d 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -197,6 +197,104 @@ def _compact_for_echo_compare(value: str) -> str: return re.sub(r'\s+', '', str(value or '')) +class _CompactEchoIndex: + """Incremental whitespace-folded index over a growing text buffer. + + The journal-rebuild echo check runs once per interim event against the + whole accumulated reasoning transcript. Rescanning the raw buffer each + time walks every whitespace character between the tail and the first + non-whitespace character — on a whitespace-heavy transcript that span is + megabytes, and the per-interim walk turns the replay quadratic (the + ``_find_compact_echo_suffix_start`` shape measured ~55x slower than + master on a production-shaped journal in the #7569 review). + + This class keeps the folded view and the raw cut offsets *incrementally*: + + * ``append`` folds each new chunk once and records, per folded character, + its raw index — so the cost of a chunk is proportional to the chunk, + never to the buffer. + * ``matches_tail`` compares only the candidate against the end of the + folded view: O(len(candidate)), with no raw-text walk at all. + * ``cut_to`` converts a folded length back to a raw index by bisecting + the recorded offsets, so one call returns the same cut point the raw + backward walk would have produced. + + ``str.isspace`` is used for folding instead of the ``\\s`` pattern used + by :func:`_compact_for_echo_compare`. The two agree on every Unicode code + point, so the indexed view and the regex-folded view stay consistent. + """ + + __slots__ = ('_compact', '_offsets', '_raw_len') + + def __init__(self) -> None: + self._compact: list[str] = [] + self._offsets: list[int] = [] + self._raw_len = 0 + + def append(self, text: str) -> None: + """Fold ``text`` onto the end of the index (one pass, no rescan).""" + raw = str(text or '') + if not raw: + return + compact = self._compact + offsets = self._offsets + base = self._raw_len + for i, ch in enumerate(raw): + if not ch.isspace(): + compact.append(ch) + offsets.append(base + i) + self._raw_len = base + len(raw) + + def reset(self) -> None: + """Drop all indexed state (used after the raw text is truncated).""" + self._compact.clear() + self._offsets.clear() + self._raw_len = 0 + + @property + def compact_length(self) -> int: + return len(self._compact) + + def matches_tail(self, suffix: str) -> bool: + """True when the folded ``suffix`` equals the folded tail.""" + candidate = _compact_for_echo_compare(suffix) + if not candidate: + return False + n = len(candidate) + if n > len(self._compact): + return False + start = len(self._compact) - n + compact = self._compact + for i in range(n): + if compact[start + i] != candidate[i]: + return False + return True + + def cut_to(self, suffix: str) -> int | None: + """Raw index at which the echo of ``suffix`` starts, else ``None``. + + The returned index is the first raw character of the echo, matching + the leftmost-cut semantics of the raw backward walk. Trailing + whitespace before the echo is left to the caller's ``rstrip``. + """ + candidate = _compact_for_echo_compare(suffix) + if not candidate: + return None + n = len(candidate) + if n > len(self._compact): + return None + start = len(self._compact) - n + compact = self._compact + for i in range(n): + if compact[start + i] != candidate[i]: + return None + return self._offsets[start] + + def compact_view(self) -> str: + """The folded text (materialized on demand — never kept as a string).""" + return ''.join(self._compact) + + def _find_compact_echo_suffix_start(value: str, suffix: str) -> int | None: """Return the index where a whitespace-folded ``suffix`` starts at the end of ``value``, or ``None`` when the tail does not echo it. @@ -10023,6 +10121,14 @@ def _run_agent_streaming( # (#3587) replaces the flat _reasoning_text string so each intermediate # assistant turn (before tool calls) keeps its own reasoning segment. _reasoning_segments: dict = {} + # Incremental folded indexes mirroring each reasoning buffer. The + # live echo strip consults them instead of re-walking the raw + # buffers: an unbounded raw scan of the growing transcript is + # quadratic on whitespace-heavy output (#7569 review). Each index + # is fed on every append and re-indexed after a strip. + _reasoning_segment_indexes: dict = {} + _stream_reasoning_index = _CompactEchoIndex() + _reasoning_buffer_index = _CompactEchoIndex() _current_reasoning_idx = 0 _tool_boundary_advanced = False _live_tool_calls = [] # tool progress fallback when final messages omit tool IDs @@ -10045,6 +10151,10 @@ def _run_agent_streaming( if _reasoning_buffer[0]: put('reasoning', {'text': _reasoning_buffer[0]}) _reasoning_buffer[0] = '' + # The folded index mirrors this buffer 1:1 — dropping the + # text without dropping the index would leave it describing + # text that is no longer there. + _reasoning_buffer_index.reset() def _emit_metering(): @@ -10084,30 +10194,37 @@ def _run_agent_streaming( nonlocal _reasoning_segments removed = False if stream_id in STREAM_REASONING_TEXT: - next_text, did_remove = _strip_compact_echo_suffix( - STREAM_REASONING_TEXT.get(stream_id, ''), - text, - ) - if did_remove: + cut = _stream_reasoning_index.cut_to(text) + if cut is not None: + next_text = STREAM_REASONING_TEXT.get(stream_id, '')[:cut].rstrip() STREAM_REASONING_TEXT[stream_id] = next_text + _stream_reasoning_index.reset() + _stream_reasoning_index.append(next_text) removed = True - next_buffer, did_remove_buffer = _strip_compact_echo_suffix(_reasoning_buffer[0], text) - if did_remove_buffer: + cut = _reasoning_buffer_index.cut_to(text) + if cut is not None: + next_buffer = _reasoning_buffer[0][:cut].rstrip() _reasoning_buffer[0] = next_buffer + _reasoning_buffer_index.reset() + _reasoning_buffer_index.append(next_buffer) removed = True for idx in (_current_reasoning_idx, _current_reasoning_idx - 1): if idx not in _reasoning_segments: continue - next_segment, did_remove_segment = _strip_compact_echo_suffix( - _reasoning_segments.get(idx, ''), - text, - ) - if not did_remove_segment: + segment_index = _reasoning_segment_indexes.get(idx) + if segment_index is None: continue + cut = segment_index.cut_to(text) + if cut is None: + continue + next_segment = _reasoning_segments.get(idx, '')[:cut].rstrip() if next_segment: _reasoning_segments[idx] = next_segment + segment_index.reset() + segment_index.append(next_segment) else: _reasoning_segments.pop(idx, None) + _reasoning_segment_indexes.pop(idx, None) removed = True break return removed @@ -10171,15 +10288,21 @@ def _run_agent_streaming( _reasoning_segments[_current_reasoning_idx] = ( _reasoning_segments.get(_current_reasoning_idx, '') + reasoning_delta ) + # Keep the folded index in step with the segment text. + _reasoning_segment_indexes.setdefault( + _current_reasoning_idx, _CompactEchoIndex() + ).append(reasoning_delta) # Mirror full concatenation to shared dict so cancel_stream() can persist # it (#1361 §A). Cancel only creates one partial message, so the flat # concatenation is correct there. # Lock-free GIL-atomic mirror — see the STREAMS_LOCK contract in on_token. if stream_id in STREAM_REASONING_TEXT: STREAM_REASONING_TEXT[stream_id] += reasoning_delta + _stream_reasoning_index.append(reasoning_delta) # Accumulate into a coalescing buffer so every delta reaches the # browser — reasoning deltas are incremental, not idempotent. _reasoning_buffer[0] += reasoning_delta + _reasoning_buffer_index.append(reasoning_delta) # Throttle reasoning SSE events to ~10 Hz to avoid overwhelming the # frontend renderer. Each event triggers _parseStreamState() which # scans the full accumulated text — 10k+ reasoning tokens/second @@ -10298,10 +10421,14 @@ def _run_agent_streaming( _reasoning_segments[_current_reasoning_idx] = ( _reasoning_segments.get(_current_reasoning_idx, '') + reason_delta ) + _reasoning_segment_indexes.setdefault( + _current_reasoning_idx, _CompactEchoIndex() + ).append(reason_delta) # Mirror full concatenation to shared dict (#1361 §A) # Lock-free GIL-atomic mirror — see STREAMS_LOCK contract in on_token. if stream_id in STREAM_REASONING_TEXT: STREAM_REASONING_TEXT[stream_id] += reason_delta + _stream_reasoning_index.append(reason_delta) put('reasoning', {'text': reason_delta}) _metering_reasoning_deltas[0] += 1 meter().record_reasoning(stream_id, _metering_reasoning_deltas[0]) diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index 1954abca..d1362548 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -135,14 +135,19 @@ and 5; it does not mark every run-state boundary implemented. transcript is stripped from the reasoning copy so the restored snapshot shows the content once. That echo match is whitespace-insensitive and carries no fixed search window: a compact-equivalent suffix is recognized - however much interior whitespace stretches its raw span - (`_find_compact_echo_suffix_start` / `_strip_compact_echo_suffix` in - `api/streaming.py`, shared with the live-stream echo path; the live journal - replay in `api/routes.py` uses the same matcher). The retired windowed - variants could drop the strip when the span exceeded the window, duplicating - the interim text; the journal-reconstruction regression - (`tests/test_live_snapshot_echo_dedup.py`) pins the single-occurrence - result. + however much interior whitespace stretches its raw span. Both consumers + (the live-stream echo path and the journal replay in `api/routes.py`) + match through an incremental folded index (`_CompactEchoIndex` in + `api/streaming.py`): the folded view and its raw cut offsets are built as + each chunk is appended, so a probe costs O(len(candidate)) and never + rescans the transcript's whitespace. The retired windowed variants could + drop the strip when the span exceeded the window, duplicating the interim + text; the retired raw backward walk was correct but re-walked the span per + interim event, quadratic on whitespace-heavy transcripts. Regressions: + `tests/test_live_snapshot_echo_dedup.py` pins the single-occurrence + result, `tests/test_live_snapshot_echo_scan_scaling.py` pins the scaling + property (a fixed-size fixture cannot catch a per-interim rescan), and + `tests/test_compact_echo_index.py` pins index/oracle equivalence. 6. **Compression is not current intent.** Automatic compression summaries and reference cards are recovery/handoff material. They must not be treated as a new user request, active-turn content, or the default visible explanation for diff --git a/tests/test_compact_echo_index.py b/tests/test_compact_echo_index.py new file mode 100644 index 00000000..bdfa431d --- /dev/null +++ b/tests/test_compact_echo_index.py @@ -0,0 +1,218 @@ +"""``_CompactEchoIndex`` must agree with the raw backward walk, cheaply. + +The index is the echo-match authority for both the journal rebuild +(``api/routes.py::_run_journal_live_snapshot``) and the live stream +(``api/streaming.py``). It replaces a per-interim raw backward walk that +rescanned the whole transcript's whitespace — quadratic on whitespace-heavy +text (#7569). Two contracts must hold for the swap to be safe: + +1. **Equivalence** — incremental appends must reach the same cut point as a + raw walk over the concatenated text, for every shape including chunked + appends, whitespace-only chunks, and exotic Unicode whitespace. +2. **Cost** — the index must never rescan the buffer it has already folded: + a probe after a wide whitespace span must not walk that span. +""" +import random +import re +import string + +import pytest + + +def _compact(value): + return re.sub(r'\s+', '', str(value or '')) + + +def _raw_reference(value, suffix): + """Deliberately naive oracle: probe every cut index, fold the tail.""" + raw = str(value or '') + candidate = _compact(suffix) + if not raw or not candidate: + return raw, False + for idx in range(len(raw) + 1): + if _compact(raw[idx:]) == candidate: + return raw[:idx].rstrip(), True + return raw, False + + +def _cases(): + cases = [ + # Exact echo at the end. + ("bla bla bla une conclusion", "une conclusion"), + # Echo differing only by whitespace shape. + ("bla bla une conclusion", "une conclusion"), + ("bla bla\nune\tconclusion", "une conclusion"), + # No echo. + ("texte quelconque sans rapport", "une conclusion"), + # Partial echo must NOT match. + ("bla une conclu", "une conclusion"), + # The buffer is exactly the echo. + ("une conclusion", "une conclusion"), + # Empty / None inputs. + ("", "une conclusion"), + ("bla bla", ""), + ("", ""), + (None, "x"), + ("x", None), + # Whitespace around the cut point. + ("bla bla une conclusion ", "une conclusion"), + ("bla bla \n une conclusion", "une conclusion"), + # Unicode, accents, emoji, exotic whitespace. + ("préambule ✅ conclusion émise", "conclusion émise"), + ("texte 🟢 Réponse", "🟢 Réponse"), + ("bla\u00a0bla une conclusion", "une\u00a0conclusion"), + ("bla\u2028une conclusion", "une conclusion"), + # Whitespace-only buffer. + (" ", "une conclusion"), + # Repetitions: the leftmost cut point is contractual. + ("abc abc abc", "abc"), + ("abc abc abc", "abc abc"), + ("aaaa", "aa"), + ] + rnd = random.Random(20260920) + alphabet = string.ascii_letters + " \n\t" + "éà✅\u00a0" + for _ in range(2000): + base = "".join(rnd.choice(alphabet) for _ in range(rnd.randint(0, 120))) + if rnd.random() < 0.5 and len(base) > 4: + k = rnd.randint(1, max(1, len(base) // 2)) + suffix = base[-k:] + else: + suffix = "".join(rnd.choice(alphabet) for _ in range(rnd.randint(0, 30))) + cases.append((base, suffix)) + return cases + + +def test_index_matches_raw_reference_oracle(): + """Incremental index == naive raw probe, for one-shot and chunked appends.""" + import api.streaming as streaming + + divergences = [] + for value, suffix in _cases(): + expected = _raw_reference(value, suffix) + index = streaming._CompactEchoIndex() + index.append(value) + actual = index.cut_to(suffix) + if actual is None: + got = (str(value or ''), False) + else: + got = (str(value or '')[:actual].rstrip(), True) + if got != expected: + divergences.append((value, suffix, expected, got)) + + assert not divergences[:5], ( + f"{len(divergences)} divergence(s) from the reference implementation; " + f"first: {divergences[:1]}" + ) + + +def test_chunked_appends_reach_the_same_cut_point(): + """Appending in pieces must equal appending the whole string.""" + import api.streaming as streaming + + rnd = random.Random(99) + for _ in range(500): + parts = [ + "".join( + rnd.choice(string.ascii_letters + " \n\té") + for _ in range(rnd.randint(1, 20)) + ) + for _ in range(rnd.randint(1, 8)) + ] + value = "".join(parts) + if rnd.random() < 0.6 and len(value) > 4: + suffix = value[-rnd.randint(1, len(value) // 2):] + else: + suffix = "".join(rnd.choice("abc \t") for _ in range(rnd.randint(0, 12))) + + chunked = streaming._CompactEchoIndex() + for part in parts: + chunked.append(part) + whole = streaming._CompactEchoIndex() + whole.append(value) + + assert chunked.cut_to(suffix) == whole.cut_to(suffix), ( + f"chunked append diverged for value={value!r} suffix={suffix!r}" + ) + + +def test_matches_tail_and_cut_to_agree(): + """The boolean probe and the cut probe must never disagree.""" + import api.streaming as streaming + + for value, suffix in _cases(): + index = streaming._CompactEchoIndex() + index.append(value) + assert index.matches_tail(suffix) == (index.cut_to(suffix) is not None), ( + f"matches_tail/cut_to disagree for value={value!r} suffix={suffix!r}" + ) + + +def test_reset_clears_state_after_truncation(): + """After reset + re-append the index must describe only the new text.""" + import api.streaming as streaming + + index = streaming._CompactEchoIndex() + index.append("old reasoning tail with a wide span " + " " * 5000) + index.reset() + index.append("fresh start only") + + assert index.compact_length == len("freshstartonly") + assert index.cut_to("start only") == len("fresh ") + assert index.cut_to("old reasoning") is None + + +def test_index_probe_does_not_rescan_a_wide_whitespace_span(monkeypatch): + """The probe must not walk raw characters it already folded. + + Pins the cost property the raw backward walk violated: after a 1 MB + whitespace span is appended, a probe must cost O(candidate), not + O(span). Counting ``str.isspace``-adjacent raw access is impractical, so + this counts how many characters ``append`` has to fold: exactly the + appended length, once — and the probe itself must not fold anything. + """ + import api.streaming as streaming + + fold_calls = {'n': 0} + original = streaming._compact_for_echo_compare + + def counting(value): + fold_calls['n'] += 1 + return original(value) + + monkeypatch.setattr(streaming, '_compact_for_echo_compare', counting) + + index = streaming._CompactEchoIndex() + index.append("alpha" + " " * 100_000) + fold_calls['n'] = 0 + + # 20 probes of a short candidate: the raw walk would fold/walk the span + # each time; the index folds only the candidate. + for i in range(20): + assert index.matches_tail(f"progress {i}") is False + + assert fold_calls['n'] <= 20, ( + f"echo probe folded {fold_calls['n']} times — it is rescanning the " + "raw transcript instead of using the indexed tail" + ) + + +@pytest.mark.timeout(60) +def test_index_stays_cheap_on_a_wide_span_with_many_probes(): + """Wide whitespace span + many probes must not cost seconds of CPU.""" + import time + + import api.streaming as streaming + + index = streaming._CompactEchoIndex() + index.append("alpha" + " " * 1_000_000) + probes = [f"progress {i}" for i in range(200)] + + start = time.perf_counter() + for probe in probes: + index.matches_tail(probe) + index.cut_to(probe) + elapsed = time.perf_counter() - start + + # The raw backward walk needs ~0.05s per probe here (2M+ character walks + # total). A generous budget still fails it by an order of magnitude. + assert elapsed < 0.5, f"200 probes on a 1MB span took {elapsed:.3f}s" diff --git a/tests/test_live_snapshot_echo_scan_scaling.py b/tests/test_live_snapshot_echo_scan_scaling.py new file mode 100644 index 00000000..6f10c4a3 --- /dev/null +++ b/tests/test_live_snapshot_echo_scan_scaling.py @@ -0,0 +1,127 @@ +"""Scaling regression for the journal-rebuild echo path (#7569). + +A fixed-size fixture cannot catch a per-interim quadratic rescan: the shape +that exposes it is a whitespace span that grows in proportion to the interim +event count, so a quadratic implementation's cost grows with the square of +the row count while a linear one stays flat per row. + +The retired implementations both rescan the raw reasoning transcript per +interim event (the folded-tail probe, then the window-free backward walk): +with a 1 MB whitespace span and 20 interim events that is 20M raw-character +walks. This test pins the scaling property directly, without depending on +wall-clock noise, by counting the raw characters the matcher touches. +""" +import json +import time + +import pytest + + +def _write_journal(root, session_id, run_id, events): + from api import run_journal as RJ + + session_root = root / RJ.RUN_JOURNAL_DIR_NAME / session_id + session_root.mkdir(parents=True, exist_ok=True) + path = session_root / f"{run_id}.jsonl" + rows = [] + for seq, (event, payload) in enumerate(events, start=1): + rows.append( + json.dumps( + { + "version": 1, + "event_id": f"{run_id}:{seq}", + "seq": seq, + "run_id": run_id, + "session_id": session_id, + "event": event, + "type": event, + "created_at": 1000 + seq, + "payload": payload, + } + ) + ) + path.write_text("\n".join(rows) + "\n", encoding="utf-8") + return path + + +def _whitespace_heavy_journal(interim_count: int, span: int): + """Reasoning ends in a wide whitespace span; every interim probes it. + + ``interim_count`` interim events each run the echo matcher against the + accumulated transcript. Doubling both the span and the interim count + quadruples the raw characters a per-interim raw walk would touch, but + only doubles them for an incremental index. + """ + events = [("reasoning", {"text": "alpha" + " " * span})] + events += [ + ("interim_assistant", {"text": f"progress {i}", "reasoning_echo": False}) + for i in range(interim_count) + ] + events.append(("token", {"text": "final answer"})) + return events + + +def _rebuild(root, run_id): + import api.routes as routes + from api import run_journal as RJ + + original = RJ._default_session_dir + RJ._default_session_dir = lambda: root + try: + return routes._run_journal_live_snapshot(run_id) + finally: + RJ._default_session_dir = original + + +@pytest.mark.parametrize("scale", [1, 2, 4]) +def test_rebuild_echo_scan_is_not_per_interim_quadratic(tmp_path, scale): + """Doubling span AND interim count must stay far under quadratic cost. + + The budget is a per-row constant: a quadratic implementation spends + ``span * interim_count`` raw-character walks, a linear one spends + ``span`` (folded once) plus ``O(interim_count * echo_len)``. The + assertion below fails the quadratic shape by roughly an order of + magnitude while leaving the linear one with generous headroom. + """ + interim_count = 20 * scale + span = 100_000 * scale + _write_journal( + tmp_path, "sess", f"run_{scale}", _whitespace_heavy_journal(interim_count, span) + ) + + start = time.perf_counter() + snapshot = _rebuild(tmp_path, f"run_{scale}") + elapsed = time.perf_counter() - start + + # Quadratic reference: 20*1 interims x 100k span took ~0.05s on the + # window-free walk, and ~1s at 1MB/20. Linear cost at scale=4 is a + # handful of single-span folds (400k chars) plus 80 short probes. + assert elapsed < 0.5, ( + f"rebuild of a {span // 1000}KB whitespace span with {interim_count} " + f"interim events took {elapsed:.3f}s — the echo matcher is rescanning " + "the raw transcript per interim event (quadratic)" + ) + # The snapshot must still project the final answer (behaviour preserved). + messages = (snapshot or {}).get("messages") or [] + assert any("final answer" in (m.get("content") or "") for m in messages) + + +def test_rebuild_scaling_is_subquadratic(tmp_path): + """Ratio check: 4x the rows must cost far less than 16x the time.""" + times = {} + for scale in (1, 2, 4): + interim_count = 20 * scale + span = 100_000 * scale + _write_journal( + tmp_path, "sess", f"run_{scale}", _whitespace_heavy_journal(interim_count, span) + ) + start = time.perf_counter() + _rebuild(tmp_path, f"run_{scale}") + times[scale] = time.perf_counter() - start + + ratio = times[4] / times[1] + assert ratio < 8.0, ( + f"4x the interim events and span cost {ratio:.1f}x the time " + f"({times}); a linear implementation stays near 4x, a quadratic one " + "reaches 16x" + ) From 5ac3632d580be89f7f052b453a9aaea53df72b1d Mon Sep 17 00:00:00 2001 From: happy5318 <5318happy@users.noreply.github.com> Date: Sun, 20 Sep 2026 20:15:24 +0800 Subject: [PATCH 27/57] chore(run-journal): drop now-unused echo helper imports from routes The journal replay matches through _CompactEchoIndex, so _find_compact_echo_suffix_start and _strip_compact_echo_suffix are no longer referenced in api/routes.py. The helpers stay in api/streaming.py as the equivalence oracle the tests compare against. --- api/routes.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/routes.py b/api/routes.py index db182efb..b315ca6f 100644 --- a/api/routes.py +++ b/api/routes.py @@ -10763,8 +10763,6 @@ from api.streaming import ( generate_session_title_for_session, _compact_for_echo_compare, _CompactEchoIndex, - _find_compact_echo_suffix_start, - _strip_compact_echo_suffix, ) from api.gateway_chat import _run_gateway_chat_streaming, webui_gateway_chat_enabled from api.run_journal import ( From 8d3c405d68207b5d13dd0ab89f086cb2b2caa63f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thireus=20=E2=98=A0?= Date: Sun, 30 Aug 2026 20:47:11 +0100 Subject: [PATCH 28/57] fix(ui): stop rendering a settled assistant answer twice (#2051) Two independent paths could put a second copy of an already-settled assistant answer on screen; the stored data always held exactly one row. 1. Dead live-turn node. The assistant row is persisted a few ms before the stream's terminal event clears S.activeStreamId, so the #6948 preserve guard still treats a finished #liveAssistantTurn node as live. Both live-turn insertion paths (renderMessages()'s re-attach and restoreLiveTurnHtmlForSession()) have one branch that APPENDS a turn the settled rebuild did not produce; the leftover node keeps id=liveAssistantTurn, is re-preserved on every later render, and the duplicate survives until a reload. 2. Settled activity scene. A process_prose row whose text IS the final answer is suppressed by the transparent-stream renderer but rebuilt as a second .assistant-segment by the compact-worklog row builder. The existing near-equality matcher never fires for short answers because of its absolute shorter>=80 floor (the reported reproduction is a 35-char greeting). Dropping a live node is now an OWNERSHIP proof, never a text comparison: the transcript must end with a settled assistant message carrying this stream's id (_anchor_stream_id / scene identity), and the node must contain nothing the settled rebuild could not have produced (no unpersisted tool card, reasoning row, or second live segment). Unknown node shapes fail closed and keep the turn, so a genuinely live turn - including one whose text matches an earlier answer - is never dropped. The settled-scene half compares raw markdown at the single chokepoint both settled renderers share. Adds tests/test_issue2051_duplicate_assistant_answer.py: four tests that fail on the pristine tree and pass with this change, including a wiring test that fails loudly if either append branch loses its guard. Fixes #2051 --- static/ui.js | 151 +++++++- ...st_issue2051_duplicate_assistant_answer.py | 338 ++++++++++++++++++ 2 files changed, 488 insertions(+), 1 deletion(-) create mode 100644 tests/test_issue2051_duplicate_assistant_answer.py diff --git a/static/ui.js b/static/ui.js index 37742c5a..ca691f42 100644 --- a/static/ui.js +++ b/static/ui.js @@ -9918,6 +9918,99 @@ function _liveAssistantSegmentTextLength(seg){ return String(body.textContent||'').trim().length; } +// ── #6948 follow-up: settled-transcript ownership of a live-turn node ────── +// (duplicate assistant answer; upstream symptom report #2051) +// +// Both live-turn insertion paths — the renderMessages() re-attach and +// restoreLiveTurnHtmlForSession() — have one branch that ADDS a turn the +// settled rebuild did not produce. The assistant row is persisted a few ms +// BEFORE the stream's terminal event clears S.activeStreamId, so a dead live +// node left behind by an INFLIGHT entry that outlived its stream is re-attached +// on top of the settled transcript: the same answer twice, self-sustaining +// across later renders (the node keeps id=liveAssistantTurn and is re-preserved +// by the #6948 guard), healed only by a reload. +// +// Dropping a live node must be an OWNERSHIP proof, never "the rendered text +// looks the same": +// * the live body is built by the streaming smd parser and the settled body +// by renderMd + post-processing, so their textContent differs for anything +// past single-paragraph plain text (lists, fenced code with its language +// label, tables, `_underscore_` emphasis, the injected copy button) — a +// text comparison would miss most real answers; and +// * an identical answer earlier in the history (a repeated "Done.", a +// greeting, a same-prompt resend) would delete a GENUINELY LIVE turn: while +// the current turn is unpersisted, the last settled assistant message IS the +// previous turn's answer. +// So ownership is proved from state: the transcript must END with a settled +// assistant message that THIS stream produced, and the node must carry nothing +// the settled rebuild could not have produced. + +// Stream identity persisted on a settled assistant message. The server stamps +// _anchor_stream_id from the anchor-scene sidecar record (api/routes.py); the +// client stamps _anchor_stream_id and scene.identity.stream_id at stream end +// (_attachProjectedAnchorSceneToLastAssistant, static/messages.js). +function _settledAssistantStreamId(message){ + if(!message) return ''; + const scene=message._anchor_activity_scene||null; + const identity=(scene&&scene.identity)||null; + return String(message._anchor_stream_id||(scene&&scene.stream_id)||(identity&&identity.stream_id)||''); +} +// The live-projection markers the #6948 preserve guard already treats as proof +// of a live owner. A message the client still considers live is never evidence +// that the turn has settled. +function _messageHasLiveAssistantProjection(m){ + return !!(m&&m.role==='assistant'&&(m._live||m._activityBurstId!==undefined||m._liveSegmentSeq!==undefined)); +} +// True when the live turn holds something the settled rebuild cannot have +// produced from S.messages — an unpersisted tool card, reasoning/thinking row, +// transparent-stream row, or a second live segment. Such a node is never +// dropped: #3714's whole premise is that the live DOM can be AHEAD of +// S.messages, and a tail-segment match must not discard the tool card or the +// earlier segment above it. An unknown node shape fails closed (keep the turn). +function _liveTurnCarriesUnsettledContent(turn){ + if(!turn||typeof turn.querySelectorAll!=='function') return true; + if(turn.querySelectorAll( + '.tool-card-row,.wl-reason,.agent-activity-thinking,.thinking-card-row,.transparent-event-row' + ).length) return true; + return turn.querySelectorAll('[data-live-assistant="1"]').length>1; +} +function _settledTranscriptOwnsLiveTurn(sid, turn){ + if(!turn) return false; + const msgs=(typeof S!=='undefined'&&S&&Array.isArray(S.messages))?S.messages:null; + if(!msgs||!msgs.length) return false; + // The transcript must END with a settled assistant answer. A trailing user + // turn means the live turn IS the current turn — nothing of it is persisted + // yet, so it can never be a leftover (this is the case a text comparison gets + // wrong when the previous answer happens to read the same), and a live + // projection anywhere means the rebuild still owns a live turn of its own. + const last=msgs[msgs.length-1]; + if(!last||last.role!=='assistant') return false; + if(msgs.some(_messageHasLiveAssistantProjection)) return false; + const inflight=(typeof INFLIGHT!=='undefined'&&INFLIGHT)?INFLIGHT[sid]:null; + // Owner of the live DOM: INFLIGHT carries the id of the stream that built it + // (attachLiveStream), and S.activeStreamId is that same id until the terminal + // event clears it. + const liveStreamId=String((inflight&&inflight.streamId)||(typeof S!=='undefined'&&S&&S.activeStreamId)||''); + const settledStreamId=_settledAssistantStreamId(last); + if(settledStreamId){ + // Identity recorded on both sides decides — renderer- and text-independent, + // so code blocks, lists and tables are handled like plain prose. + if(!liveStreamId||settledStreamId!==liveStreamId) return false; + }else{ + // No persisted identity on the tail (a turn whose scene was not worklog + // worthy). Compare SOURCE with SOURCE — the markdown this stream produced + // (INFLIGHT.lastAssistantText) against the persisted message content — + // never two independently rendered DOM trees. Regeneration cannot reach + // this branch: startRegeneration() truncates S.messages at the user turn, + // so the tail is a user message while a regenerated answer streams. + const streamed=String((inflight&&inflight.lastAssistantText)||'').replace(/\s+/g,' ').trim(); + if(!streamed) return false; + const settled=String((typeof msgContent==='function'?msgContent(last):last.content)||'').replace(/\s+/g,' ').trim(); + if(!settled||streamed!==settled) return false; + } + return !_liveTurnCarriesUnsettledContent(turn); +} + function _mergeRestoredLiveAssistantSegment(restored, existing){ if(!restored||!existing) return; const existingLive=existing.querySelector('[data-live-assistant="1"]'); @@ -9954,6 +10047,18 @@ function restoreLiveTurnHtmlForSession(sid){ const existing=$('liveAssistantTurn'); _mergeRestoredLiveAssistantSegment(restored, existing); if(existing) existing.replaceWith(restored); + // #6948 follow-up (duplicate assistant answer; #2051): only this branch ADDS a + // turn — replacing an existing live turn cannot duplicate. When the settled + // transcript already owns the stream this snapshot belongs to, appending it + // pins a SECOND copy of the same answer under the settled one. Release the + // stale snapshot (no other consumer can use it — it would be re-refused on + // every later restore) and report "nothing restored", so loadSession takes the + // same path it already takes for an INFLIGHT entry that never carried one. + else if(typeof _settledTranscriptOwnsLiveTurn==='function' + &&_settledTranscriptOwnsLiveTurn(sid, restored)){ + inflight.liveTurnHtml=null; + return false; + } else inner.appendChild(restored); // Transparent Stream: liveTurnHtml is restored via template.innerHTML, which // drops the property-bound onclick/onkeydown handlers wired by @@ -13141,6 +13246,17 @@ function _anchorSceneRowsForRendering(scene, opts){ const rows=Array.isArray(scene&&scene.activity_rows)?scene.activity_rows:[]; const settled=!!(opts&&opts.settled); const live=!settled; + // #6948 follow-up (duplicate assistant answer; #2051): on a SETTLED turn the + // assistant segment owns the final answer, so a process_prose row carrying + // that same text must not also be rebuilt into the activity scene. The + // transparent-stream renderer suppresses it per row + // (_anchorSceneTransparentNodeForRow), but the compact-worklog row builder + // (_anchorSceneNodeForRow) has no such guard and renders the row as a second + // .assistant-segment above the settled one. Drop it here — the single place + // BOTH settled renderers (and the #5839 deferred-rows path) get their rows. + // LIVE rendering is untouched: while streaming, the inline live segment is + // hidden and the prose row IS the visible answer. + const settledFinalAnswer=settled?String((scene&&scene.final_answer)||'').trim():''; const out=[]; const byKey=new Map(); const liveProseTextKeys=new Map(); @@ -13163,6 +13279,9 @@ function _anchorSceneRowsForRendering(scene, opts){ if(_anchorSceneIsSettledSuccessfulCompression(row,settled)) continue; const text=String(row.text||'').trim(); if((row.role==='prose'||row.role==='thinking')&&!text) continue; + if(settledFinalAnswer&&row.role==='prose' + &&typeof _anchorSceneProseDuplicatesFinalAnswer==='function' + &&_anchorSceneProseDuplicatesFinalAnswer(text,settledFinalAnswer)) continue; // #6948 follow-up const key=keyFor(row); if(byKey.has(key)){ const index=byKey.get(key); @@ -13437,6 +13556,26 @@ function _anchorSceneProseMatchesFinalAnswer(proseText, finalAnswer){ const shorter=Math.min(a.length,b.length), longer=Math.max(a.length,b.length); return shorter>=80 && (shorter/longer)>=0.9; } +// #6948 follow-up (duplicate assistant answer; #2051): the same near-equality +// test as _anchorSceneProseMatchesFinalAnswer, minus its absolute `shorter>=80` +// floor. That floor makes the matcher a no-op for every SHORT final answer +// (greetings, "Done.", one-liners), so a prose row that is the answer minus its +// last streamed token — e.g. "Hey! What can I help you with today" vs +// "...today?" — was rebuilt as a SECOND assistant-segment beside the settled +// one. The >=0.9 length ratio is what actually keeps a distinct short +// intermediate sentence from being swallowed by a long final answer (Codex +// #4568), so it alone is kept. Deliberately a separate helper: the per-row +// render-time matcher above, and the tests pinning its exact predicate, stay +// untouched. +function _anchorSceneProseDuplicatesFinalAnswer(proseText, finalAnswer){ + if(_anchorSceneProseMatchesFinalAnswer(proseText,finalAnswer)) return true; + const norm=(s)=>String(s||'').replace(/\s+/g,' ').trim(); + const a=norm(proseText), b=norm(finalAnswer); + if(!a||!b) return false; + if(!(a.startsWith(b)||b.startsWith(a))) return false; + const shorter=Math.min(a.length,b.length), longer=Math.max(a.length,b.length); + return (shorter/longer)>=0.9; +} function _anchorSceneWorklogGroup(blocks, opts){ if(!blocks) return null; const live=!!(opts&&opts.live); @@ -18593,7 +18732,17 @@ function renderMessages(options){ // restore the whole preserved turn so nothing the user saw vanishes. if(S.session) _preservedLiveTurn.dataset.sessionId=S.session.session_id; _rebuilt.replaceWith(_preservedLiveTurn); - }else{ + }else if(!(typeof _settledTranscriptOwnsLiveTurn==='function' + &&_settledTranscriptOwnsLiveTurn(sid,_preservedLiveTurn))){ + // #6948 follow-up (duplicate assistant answer; #2051): this is the + // only branch that ADDS a turn — the rebuild produced no live turn of + // its own. When the settled transcript already ends with THIS stream's + // own answer and the preserved node carries nothing unpersisted, the + // node is a dead leftover (the row persisted and the turn settled while + // INFLIGHT[sid] was not yet cleaned) and appending pins a SECOND copy + // that re-preserves itself on every later render until a reload. + // Mid-stream the transcript ends with the user turn (or still carries a + // live projection), so #3877 preservation is untouched. if(S.session) _preservedLiveTurn.dataset.sessionId=S.session.session_id; inner.appendChild(_preservedLiveTurn); } diff --git a/tests/test_issue2051_duplicate_assistant_answer.py b/tests/test_issue2051_duplicate_assistant_answer.py new file mode 100644 index 00000000..6493bf16 --- /dev/null +++ b/tests/test_issue2051_duplicate_assistant_answer.py @@ -0,0 +1,338 @@ +"""Regression: the assistant answer is displayed TWICE (#6948 follow-up; +upstream symptom report #2051). + +Two independent renderers can put a second copy of the same answer on screen +after a turn settles. Both are covered here. + +1. The live-turn node (static/ui.js) +------------------------------------ +`renderMessages()` re-attaches the preserved `#liveAssistantTurn` node after the +transcript wipe, and `restoreLiveTurnHtmlForSession()` re-inserts the +`INFLIGHT[sid].liveTurnHtml` snapshot on a session switch / reconnect. Each has +ONE branch that *appends* — i.e. adds a turn the settled rebuild did not +produce. The assistant row is persisted a few ms BEFORE the stream's terminal +event clears `S.activeStreamId`, so a dead live node left behind by an INFLIGHT +entry that outlived its stream is appended under the settled copy: the same +answer twice, self-sustaining across later renders (the node keeps +id=liveAssistantTurn and is re-preserved by the #6948 guard), healed only by a +reload. + +The contract (`_settledTranscriptOwnsLiveTurn`) is an OWNERSHIP proof, never a +rendered-text comparison: + + * the live body comes from the streaming smd parser and the settled body from + renderMd + post-processing, so comparing textContent would only ever match + single-paragraph plain text — lists, fenced code, tables and `_emphasis_` + render differently in the two pipelines; + * comparing text would DELETE a genuinely live turn whenever the previous + answer reads the same ("Done.", a greeting, a same-prompt resend), because + while the current turn is unpersisted the last settled assistant message is + the PREVIOUS turn's answer. + +So the append is skipped only when the transcript ENDS with a settled assistant +message that the SAME stream produced (persisted anchor identity, or a +source-vs-source text match when the turn has no persisted scene), and the +preserved node carries nothing the settled rebuild could not have produced (no +unpersisted tool card / reasoning row, no second live segment — #3714). + +2. The settled activity scene (static/ui.js) +-------------------------------------------- +`_anchorSceneRowsForRendering(scene,{settled:true})` feeds both settled +renderers. A `process_prose` row whose text IS the final answer is suppressed +per-row by the transparent-stream renderer but NOT by the compact-worklog row +builder, which rebuilds it as a second `.assistant-segment` above the settled +answer. The row is now dropped in the shared filter, using a near-equality test +without the absolute `shorter>=80` floor that made the existing matcher a no-op +for short answers (the reported case: the row is the answer minus its last +streamed token, 35 vs 36 chars). LIVE rendering is untouched — while streaming, +the inline live segment is hidden and the prose row IS the visible answer. +""" +import shutil +import subprocess +from pathlib import Path + +import pytest + +from tests.js_source_extract import extract_function + +ROOT = Path(__file__).resolve().parents[1] +UI_JS = (ROOT / "static" / "ui.js").read_text(encoding="utf-8") +NODE = shutil.which("node") + + +def js(*names): + """Real source of the named ui.js functions — never a re-implementation.""" + return "\n".join(extract_function(UI_JS, name) for name in names) + + +def run_node(script): + out = subprocess.run([NODE, "-e", script], capture_output=True, text=True) + assert out.returncode == 0, f"node harness failed:\n{out.stderr}\n{out.stdout}" + assert "OK" in out.stdout, out.stdout + return out.stdout + + +# Minimal DOM stand-in: class/attribute selector matching over a node tree. +DOM_STUB = """ +const assert=require('assert'); +class El{ + constructor(spec){ + spec=spec||{}; + this.classes=new Set(spec.classes||[]); + this.attrs=Object.assign({},spec.attrs||{}); + this.children=spec.children||[]; + } + _matchesToken(tok){ + tok=String(tok).trim(); + if(tok.startsWith('.')) return this.classes.has(tok.slice(1)); + const m=/^\\[([a-zA-Z0-9-]+)="([^"]*)"\\]$/.exec(tok); + if(m) return String(this.attrs[m[1]]===undefined?'':this.attrs[m[1]])===m[2]; + return false; + } + querySelectorAll(sel){ + const toks=String(sel).split(','); + const out=[]; + const walk=(node)=>{ + for(const child of node.children){ + if(toks.some(t=>child._matchesToken(t))) out.push(child); + walk(child); + } + }; + walk(this); + return out; + } +} +const seg=(text)=>new El({attrs:{'data-live-assistant':'1'},classes:['assistant-segment'], + children:[new El({classes:['msg-body'],children:[]})]}); +const turnWith=(...kids)=>new El({classes:['assistant-turn'],children:kids}); +const toolCard=()=>new El({classes:['tool-card-row']}); +const emptyWorklogShell=()=>new El({classes:['live-worklog'],attrs:{'data-live-worklog-shell':'1'}}); +var S={messages:[],activeStreamId:null,session:{session_id:'s1'}}; +var INFLIGHT={}; +function setState(messages,inflight,activeStreamId){ + S.messages=messages; + S.activeStreamId=activeStreamId===undefined?null:activeStreamId; + for(const k of Object.keys(INFLIGHT)) delete INFLIGHT[k]; + if(inflight) INFLIGHT['s1']=inflight; +} +const user=(content)=>({role:'user',content}); +const settledAnswer=(content,streamId)=>{ + const m={role:'assistant',content}; + if(streamId) m._anchor_stream_id=streamId; + return m; +}; +const sceneIdentityAnswer=(content,streamId)=>({ + role:'assistant',content, + _anchor_activity_scene:{identity:{stream_id:streamId},final_answer:content}, +}); +""" + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_dead_live_turn_is_dropped_only_when_the_stream_is_provably_settled(): + """The #2051 duplicate is suppressed; a live turn is never deleted.""" + script = DOM_STUB + js( + "msgContent", + "_settledAssistantStreamId", + "_messageHasLiveAssistantProjection", + "_liveTurnCarriesUnsettledContent", + "_settledTranscriptOwnsLiveTurn", + ) + """ +const ANSWER='Hey! What can I help you with today?'; + +// ── the bug: stream settled, INFLIGHT not yet cleaned → dead leftover ── +setState([user('Hi'),settledAnswer(ANSWER,'st1')],{streamId:'st1'},'st1'); +assert.strictEqual( + _settledTranscriptOwnsLiveTurn('s1',turnWith(seg(ANSWER),emptyWorklogShell())),true, + 'dead live node whose stream the settled transcript owns must be dropped (#2051)'); + +// Identity may live only on the persisted scene. +setState([user('Hi'),sceneIdentityAnswer(ANSWER,'st1')],{streamId:'st1'},null); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg(ANSWER))),true, + 'scene identity.stream_id is accepted as ownership proof'); + +// Shape independence: the decision must not depend on rendered text at all, so +// a multi-block answer (fenced code / list / table), which the smd parser and +// renderMd render to DIFFERENT textContent, is handled like plain prose. +setState([user('Hi'),settledAnswer('Here you go:\\n\\n```python\\nprint(1)\\n```','st1')], + {streamId:'st1'},'st1'); +assert.strictEqual( + _settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Here you go:print(1)'))),true, + 'a code-block answer must be de-duplicated too (renderer-independent)'); + +// ── false positives that a text comparison would cause ── +// The current turn is unpersisted: the transcript ends with the USER turn while +// the live turn streams text identical to the PREVIOUS answer. +setState([user('Hi'),settledAnswer('Done.','st1'),user('Hi')],{streamId:'st2'},'st2'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Done.'))),false, + 'a genuinely live turn must never be dropped because an earlier answer matches'); +// markInflight() shape at send time (no snapshot yet, stream just started). +setState([user('Hi'),settledAnswer('Done.','st1'),user('Hi')],{sid:'s1',streamId:'st2',ts:1},'st2'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Do'))),false, + 'partial live text under a fresh stream must never be dropped'); +// A settled tail from a DIFFERENT stream never owns this live turn. +setState([user('Hi'),settledAnswer('Done.','st1')],{streamId:'st2'},'st2'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Done.'))),false, + 'a tail from another stream must not authorize dropping the live turn'); +// Reconnect/terminal projection: S.messages still carries a live marker. +setState([user('Hi'),Object.assign(settledAnswer('Done.','st1'),{_live:true})], + {streamId:'st1'},null); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Done.'))),false, + 'a live projection in S.messages means the turn has not settled'); +// Ownership unknown (no INFLIGHT stream id, no active stream) → fail closed. +setState([user('Hi'),settledAnswer('Done.','st1')],{},null); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Done.'))),false, + 'unknown live owner must fail closed'); +setState([],{streamId:'st1'},'st1'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Done.'))),false, + 'an empty transcript owns nothing'); + +// ── #3714: live-only structure is never discarded ── +setState([user('Hi'),settledAnswer(ANSWER,'st1')],{streamId:'st1'},'st1'); +assert.strictEqual( + _settledTranscriptOwnsLiveTurn('s1',turnWith(toolCard(),seg(ANSWER))),false, + 'a preserved turn carrying an unpersisted tool card must be kept'); +assert.strictEqual( + _settledTranscriptOwnsLiveTurn('s1',turnWith(seg('first'),seg(ANSWER))),false, + 'a multi-segment live turn must not be dropped on a tail-segment match'); +assert.strictEqual( + _settledTranscriptOwnsLiveTurn('s1',turnWith(new El({classes:['wl-reason']}),seg(ANSWER))),false, + 'a preserved turn carrying an unpersisted reasoning row must be kept'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',{}),false, + 'an unknown node shape must fail closed'); + +// ── no persisted scene identity: SOURCE text vs SOURCE text ── +setState([user('Hi'),settledAnswer(ANSWER,null)],{streamId:'st1',lastAssistantText:ANSWER},'st1'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg(ANSWER))),true, + 'streamed markdown identical to the persisted answer proves ownership'); +setState([user('Hi'),settledAnswer(ANSWER,null)], + {streamId:'st1',lastAssistantText:'Hey! What can I help'},'st1'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg('Hey! What can I help'))),false, + 'a still-growing stream must not be dropped against a longer settled answer'); +setState([user('Hi'),settledAnswer(ANSWER,null)],{streamId:'st1'},'st1'); +assert.strictEqual(_settledTranscriptOwnsLiveTurn('s1',turnWith(seg(ANSWER))),false, + 'no identity and no streamed source text → fail closed'); +console.log('OK'); +""" + run_node(script) + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_append_decision_renders_the_answer_exactly_once(): + """Observable outcome of the two append branches, driven by the real + predicate: one copy for a settled leftover, and the live turn is still + attached in every case where the turn may still be running.""" + script = DOM_STUB + js( + "msgContent", + "_settledAssistantStreamId", + "_messageHasLiveAssistantProjection", + "_liveTurnCarriesUnsettledContent", + "_settledTranscriptOwnsLiveTurn", + ) + """ +// Mirrors the two call sites: the branch appends unless the settled transcript +// provably owns the node (static/ui.js renderMessages re-attach and +// restoreLiveTurnHtmlForSession). +function visibleAnswers(messages,inflight,activeStreamId,turn){ + setState(messages,inflight,activeStreamId); + const settledCopies=messages.filter(m=>m.role==='assistant'&&!m._live).length; + const appended=!_settledTranscriptOwnsLiveTurn('s1',turn)?1:0; + return {settledCopies,appended,total:settledCopies+appended}; +} +const ANSWER='Hey! What can I help you with today?'; +// #2051: exactly one copy of the answer. +assert.deepStrictEqual( + visibleAnswers([user('Hi'),settledAnswer(ANSWER,'st1')],{streamId:'st1'},'st1', + turnWith(seg(ANSWER),emptyWorklogShell())), + {settledCopies:1,appended:0,total:1}, + 'the settled answer must render exactly once'); +// A live turn is still attached while the turn is running (#3877). +assert.deepStrictEqual( + visibleAnswers([user('Hi'),settledAnswer(ANSWER,'st1'),user('Hi again')], + {streamId:'st2'},'st2',turnWith(seg(ANSWER))), + {settledCopies:1,appended:1,total:2}, + 'a running turn must stay on screen even when it repeats an earlier answer'); +// A leftover carrying unpersisted structure is kept whole (#3714). +assert.deepStrictEqual( + visibleAnswers([user('Hi'),settledAnswer(ANSWER,'st1')],{streamId:'st1'},'st1', + turnWith(toolCard(),seg(ANSWER))).appended,1, + 'never discard live-only blocks the rebuild did not produce'); +console.log('OK'); +""" + run_node(script) + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_settled_scene_drops_the_prose_row_that_is_the_final_answer(): + script = DOM_STUB + js( + "_anchorSceneIsSettledSuccessfulCompression", + "_anchorSceneProseMatchesFinalAnswer", + "_anchorSceneProseDuplicatesFinalAnswer", + "_anchorSceneRowsForRendering", + ) + """ +function _anchorSceneToolRowLogicalKey(row){ return (row&&row.row_id)||''; } +function _anchorSceneMergeToolRows(_prev,row){ return row; } +const FINAL='Hey! What can I help you with today?'; +const scene={ + final_answer:FINAL, + activity_rows:[ + {role:'thinking',row_id:'t1',text:'thinking out loud'}, + {role:'prose',row_id:'p1',text:'Let me check the config first.'}, + // the reported shape: the answer minus its last streamed token (35 vs 36) + {role:'prose',row_id:'p2',text:'Hey! What can I help you with today'}, + {role:'terminal',row_id:'d1',source_event_type:'done'}, + ], +}; +const texts=(opts)=>_anchorSceneRowsForRendering(scene,opts).map(r=>r.text); +assert.deepStrictEqual(texts({settled:true}),['thinking out loud','Let me check the config first.'], + 'the settled scene must not rebuild the final answer as a second segment (#2051)'); +assert.deepStrictEqual(texts({settled:false}), + ['thinking out loud','Let me check the config first.','Hey! What can I help you with today'], + 'LIVE rendering must keep the prose row — it IS the visible answer while streaming'); +// Exact repeats and multi-block answers are dropped too; source text is compared, +// so the markdown pipeline never matters here. +const code={final_answer:'Here you go:\\n\\n```python\\nprint(1)\\n```',activity_rows:[ + {role:'prose',row_id:'p1',text:'Here you go:\\n\\n```python\\nprint(1)\\n```'}, + {role:'prose',row_id:'p2',text:'Working on it.'}]}; +assert.deepStrictEqual(_anchorSceneRowsForRendering(code,{settled:true}).map(r=>r.text), + ['Working on it.'],'an exact fenced-code duplicate must be dropped, intermediates kept'); +// Codex #4568: a SHORT distinct intermediate that merely prefixes a LONG answer +// must survive — the >=0.9 length ratio, not the removed absolute floor, is what +// protects it. +const long_={final_answer:'Hey! What can I help you with today? I can read files, run commands and edit code.', + activity_rows:[{role:'prose',row_id:'p1',text:'Hey! What can'}]}; +assert.deepStrictEqual(_anchorSceneRowsForRendering(long_,{settled:true}).map(r=>r.text), + ['Hey! What can'],'a distinct short intermediate must not be swallowed by a long answer'); +// The pinned per-row matcher is untouched: its absolute floor still rejects the +// short near-miss that the new helper accepts. +assert.strictEqual(_anchorSceneProseMatchesFinalAnswer('Hey! What can I help you with today',FINAL),false, + '_anchorSceneProseMatchesFinalAnswer must keep its >=80 char floor'); +assert.strictEqual(_anchorSceneProseDuplicatesFinalAnswer('Hey! What can I help you with today',FINAL),true, + 'the new helper must catch the short near-miss'); +assert.strictEqual(_anchorSceneProseDuplicatesFinalAnswer('Hey! What can',FINAL),false, + 'the new helper must keep the >=0.9 ratio guard'); +console.log('OK'); +""" + run_node(script) + + +def test_both_append_branches_are_gated_by_the_ownership_check(): + """Wiring: the predicate only helps if it guards the two branches that ADD a + turn (the replace branches cannot duplicate and must stay unguarded).""" + restore = UI_JS[UI_JS.index("function restoreLiveTurnHtmlForSession"):] + restore = restore[:restore.index("\nfunction markInflight")] + assert "if(existing) existing.replaceWith(restored);" in restore + assert "_settledTranscriptOwnsLiveTurn(sid, restored)" in restore, ( + "the restore append branch must be gated by the ownership check" + ) + assert "inflight.liveTurnHtml=null;" in restore, ( + "a refused snapshot must be released, or it is re-refused on every restore" + ) + reattach = UI_JS[UI_JS.index(" const _rebuilt=document.getElementById('liveAssistantTurn');"):] + reattach = reattach[:reattach.index(" // Only force-scroll when not actively streaming")] + assert "_settledTranscriptOwnsLiveTurn(sid,_preservedLiveTurn)" in reattach, ( + "the renderMessages append branch must be gated by the ownership check" + ) + assert reattach.count("_settledTranscriptOwnsLiveTurn(") == 1, ( + "the segment-swap and whole-turn replace branches must stay unguarded — " + "they replace a node instead of adding one and cannot duplicate" + ) From 166ca0ec05485e640a32018d3a183fc0151b845a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thireus=20=E2=98=A0?= Date: Tue, 1 Sep 2026 12:07:15 +0100 Subject: [PATCH 29/57] fix(ui): build message blocks as nodes, not from an HTML string (#2051) A settled assistant answer could render TWICE inside a single `.assistant-segment` -- `msg-body, msg-foot, msg-body, msg-foot`, the two copies byte-identical -- while the session data, the server responses and the emitted markup were all correct. It survived reloads, was unreproducible on a clean machine, and could not be explained by any render path. The cause is outside the app. Privacy / anti-fingerprinting extensions wrap the DOM insertion APIs so they can re-scan the document after every mutation. The reporter identified CanvasBlocker 1.12 in their own profile; its `lib/iframeProtection.js` -> `protectDOMModifications()` wraps `appendChild`, `insertBefore` and `replaceChild` on Node.prototype, `append`, `prepend`, `insertAdjacentElement`, `insertAdjacentHTML`, `insertAdjacentText` and `replaceWith` on Element.prototype, and the `innerHTML` and `outerHTML` setters, and installs them with `exportFunction`, so they still stringify as `[native code]` and cannot be told apart from the real thing by the page. A wrapper that ends up executing the wrapped call twice is harmless for node-based insertion but not for string-based insertion. Measured on the reporter's exact 2342-char emit string, in Chromium 152 and Firefox 154: a doubled `insertAdjacentHTML` rendered 2 bodies before this change and 1 after, while a doubled `appendChild`, `insertBefore`, `append` or `innerHTML` setter rendered 1 body both before and after. Only the HTML-string path multiplies: `insertAdjacentHTML` parses and appends a fresh copy every time it runs, whereas appending a node MOVES it (a second call is a no-op) and an `innerHTML` setter REPLACES (a second call is idempotent). That is why exactly this one construct, of all the wrapped APIs, produced the duplicate -- and why switching to a different insertion API is a real fix rather than a workaround for one extension. Parse into a `