diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index aae3d695..408fd3e0 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -210,14 +210,17 @@ writes. A client still running the previous revision therefore does not observe clears recorded after the migration; that reload boundary is deliberate, because a dual write cannot make the shared blob concurrency-safe. Versioned records are - pruned by age (7 days) and supersession only — never by session existence, - because the sidebar list is filtered by profile, project, and source, so an - absent row may simply be hidden. They are never part of the marker map - consumers read. A clear order is retained in module memory even when storage - quota prevents allocating its versioned key; the client still attempts the - smaller write that removes the marker from the existing marker map, so a user - can dismiss unread state under storage pressure. Logical clear order and - retention time are separate: records compare markers using their order stamp, + pruned by age (7 days), or when a newer ordering fact for that session was + successfully persisted — never by an in-memory-only superseding clear and never + by session existence, because the sidebar list is filtered by profile, project, + and source, so an absent row may simply be hidden. They are never part of the + marker map consumers read. A clear order is retained in module memory even when + storage quota prevents allocating its versioned key; the client still attempts + the smaller write that removes the marker from the existing marker map, so a user + can dismiss unread state under storage pressure. The failed allocation also + leaves any older durable clear record in place so reload does not lose the last + persisted ordering fact. Logical clear order and retention time are separate: + records compare markers using their order stamp, but the 7-day cap uses the wall-clock time at which the record was written, so a future logical stamp cannot extend retention indefinitely. The in-memory fallback lasts until reload, while successfully persisted records retain the diff --git a/static/sessions.js b/static/sessions.js index 8efd006d..ae6bc699 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -744,10 +744,10 @@ function _parseSessionCompletionUnreadClearedKey(key) { return { sid, version }; } -// Read the legacy whole-map key plus independent versioned clear records. A -// clear writes a new key rather than replacing an older operation, so readers -// can fold the maximum timestamp without a cross-context compare-and-set. -function _readSessionCompletionUnreadCleared() { +// Read persisted clear ordering independently from the in-memory fallback. The +// distinction matters during pruning: a higher clear that failed quota and lives +// only in memory must not delete the older durable record it supersedes. +function _readPersistedSessionCompletionUnreadCleared() { // Snapshot keys before values. A clear that starts after this operation began // is deliberately excluded, so a marker prepared first gets the same logical // order and loses the clear-wins tie. A marker started after a completed clear @@ -757,16 +757,6 @@ function _readSessionCompletionUnreadCleared() { keys = Array.from({ length: localStorage.length }, (_, i) => localStorage.key(i)); } catch (_) {} const cleared = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); - const now = Date.now(); - for (const [sid, memoryRecord] of Object.entries(_sessionCompletionUnreadClearedMemory || {})) { - const value = Number(memoryRecord && memoryRecord.order) || 0; - const recordedAt = Number(memoryRecord && memoryRecord.recorded_at) || 0; - if (recordedAt && now - recordedAt > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS) { - delete _sessionCompletionUnreadClearedMemory[sid]; - continue; - } - if (value > (Number(cleared[sid]) || 0)) cleared[sid] = value; - } try { for (const key of keys) { const parsed = _parseSessionCompletionUnreadClearedKey(key); @@ -781,6 +771,23 @@ function _readSessionCompletionUnreadCleared() { return cleared; } +// Combine durable clear ordering with this client's in-memory fallback. The +// fallback keeps the current window correct when quota rejects a new key. +function _readSessionCompletionUnreadCleared() { + const cleared = _readPersistedSessionCompletionUnreadCleared(); + const now = Date.now(); + for (const [sid, memoryRecord] of Object.entries(_sessionCompletionUnreadClearedMemory || {})) { + const value = Number(memoryRecord && memoryRecord.order) || 0; + const recordedAt = Number(memoryRecord && memoryRecord.recorded_at) || 0; + if (recordedAt && now - recordedAt > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS) { + delete _sessionCompletionUnreadClearedMemory[sid]; + continue; + } + if (value > (Number(cleared[sid]) || 0)) cleared[sid] = value; + } + return cleared; +} + function _nextSessionCompletionUnreadOrder(sid, cleared = null) { const clears = cleared || _readSessionCompletionUnreadCleared(); const cached = _getSessionCompletionUnread()[sid]; @@ -931,7 +938,7 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { try { localStorage.removeItem(SESSION_COMPLETION_UNREAD_CLEARED_KEY); } catch (_) {} } - const cleared = _readSessionCompletionUnreadCleared(); + const persistedCleared = _readPersistedSessionCompletionUnreadCleared(); let keys = []; try { keys = Array.from({ length: localStorage.length }, (_, i) => localStorage.key(i)); @@ -943,11 +950,10 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { const recordedAt = Number(localStorage.getItem(key)) || 0; const version = parsed.version || recordedAt; const expired = recordedAt > 0 && Date.now() - recordedAt > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS; - const superseded = version < (Number(cleared[parsed.sid]) || 0); - // Records are pruned by age and supersession only. Session existence is - // not a pruning signal: the sidebar list is filtered by profile, project, - // and source, so a session missing from it may simply be hidden, and - // dropping its clear record would re-light a cleared dot. + const superseded = version < (Number(persistedCleared[parsed.sid]) || 0); + // Prune by age, or when a newer ordering fact is itself durable. A + // memory-only clear may keep this window correct under quota pressure, but + // it cannot safely replace the older record across reload. if (expired || superseded) localStorage.removeItem(key); } catch (_) {} } diff --git a/tests/_unread_store_helpers.py b/tests/_unread_store_helpers.py index 0d187e64..ed31e43d 100644 --- a/tests/_unread_store_helpers.py +++ b/tests/_unread_store_helpers.py @@ -32,6 +32,7 @@ _HELPER_NAMES = ( "_nextSessionCompletionUnreadOrder", "_sessionCompletionUnreadClearedKey", "_parseSessionCompletionUnreadClearedKey", + "_readPersistedSessionCompletionUnreadCleared", "_readSessionCompletionUnreadCleared", "_mergeSessionCompletionUnread", "_writeSessionCompletionUnreadCleared", diff --git a/tests/test_issue856_background_completion_unread.py b/tests/test_issue856_background_completion_unread.py index f11349ba..83dcc58a 100644 --- a/tests/test_issue856_background_completion_unread.py +++ b/tests/test_issue856_background_completion_unread.py @@ -1,12 +1,21 @@ """Regression checks for #856 background completion unread markers.""" import json +import shutil +import subprocess +import sys from pathlib import Path +import pytest + REPO = Path(__file__).resolve().parent.parent SESSIONS_JS = (REPO / "static" / "sessions.js").read_text(encoding="utf-8") MESSAGES_JS = (REPO / "static" / "messages.js").read_text(encoding="utf-8") +NODE = shutil.which("node") + +sys.path.insert(0, str(REPO / "tests")) +import _unread_store_helpers as unread_store_helpers # noqa: E402 def _done_block() -> str: @@ -40,25 +49,81 @@ def _function_body(block: str) -> str: raise AssertionError("function closing brace not found") +def _extract_sessions_function(name: str) -> str: + start = SESSIONS_JS.index(f"function {name}(") + brace = SESSIONS_JS.index("{", start) + depth = 0 + for i in range(brace, len(SESSIONS_JS)): + if SESSIONS_JS[i] == "{": + depth += 1 + elif SESSIONS_JS[i] == "}": + depth -= 1 + if depth == 0: + return SESSIONS_JS[start:i + 1] + raise AssertionError(f"could not brace-match {name}") + + +def _run_unread_behavior(body: str) -> dict: + functions = "\n".join( + _extract_sessions_function(name) + for name in ( + "_getSessionViewedCounts", + "_saveSessionViewedCounts", + "_setSessionViewedCount", + "_getSessionCompletionUnread", + "_saveSessionCompletionUnread", + "_clearSessionCompletionUnread", + "_hasSessionCompletionUnread", + "_hasUnreadForSession", + ) + ) + script = f""" +const store = {{}}; +const localStorage = {{ + getItem: (key) => Object.prototype.hasOwnProperty.call(store, key) ? store[key] : null, + setItem: (key, value) => {{ store[key] = String(value); }}, + removeItem: (key) => {{ delete store[key]; }}, + key: (index) => Object.keys(store)[index] ?? null, + get length() {{ return Object.keys(store).length; }}, +}}; +const SESSION_VIEWED_COUNTS_KEY = 'hermes-session-viewed-counts'; +const SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'; +let _sessionViewedCounts = null; +let _sessionCompletionUnread = null; +let _allSessions = [{{session_id: 'X'}}]; +const _sessionListSnapshotById = new Map(); +const S = {{session: null}}; +{unread_store_helpers.BLOCK} +{functions} +{body} +""" + result = subprocess.run( + [NODE, "-e", script], capture_output=True, text=True, timeout=60 + ) + assert result.returncode == 0, f"node harness failed:\n{result.stderr}" + return json.loads(result.stdout) + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") def test_background_completion_unread_uses_explicit_marker_not_message_delta(): """A background completion must stay unread even when message_count has no delta.""" - assert "SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'" in SESSIONS_JS - assert "function _markSessionCompletionUnread(" in SESSIONS_JS - assert "function _clearSessionCompletionUnread(" in SESSIONS_JS - assert "function _hasSessionCompletionUnread(" in SESSIONS_JS - - has_unread_idx = SESSIONS_JS.find("function _hasUnreadForSession(s)") - assert has_unread_idx != -1, "_hasUnreadForSession not found" - has_unread_block = SESSIONS_JS[has_unread_idx:SESSIONS_JS.find("async function newSession", has_unread_idx)] - - marker_idx = has_unread_block.find("_hasSessionCompletionUnread(s.session_id)") - count_idx = has_unread_block.find("s.message_count > viewed.message_count") - assert marker_idx != -1, "_hasUnreadForSession must check explicit completion unread marker" - assert count_idx != -1, "_hasUnreadForSession must keep the existing message_count fallback" - assert marker_idx < count_idx, ( - "explicit completion unread marker must be checked before message_count delta, " - "because completed streams can have viewed_count == message_count" - ) + out = _run_unread_behavior(""" +store[SESSION_VIEWED_COUNTS_KEY] = JSON.stringify({ + X: {message_count: 3, transcript_generation: 0}, +}); +store[SESSION_COMPLETION_UNREAD_KEY] = JSON.stringify({ + X: {message_count: 3, completed_at: 1000, unread_order: 1000}, +}); +const beforeClear = _hasUnreadForSession({ + session_id: 'X', message_count: 3, transcript_generation: 0, +}); +_clearSessionCompletionUnread('X'); +const afterClear = _hasUnreadForSession({ + session_id: 'X', message_count: 3, transcript_generation: 0, +}); +console.log(JSON.stringify({beforeClear, afterClear})); +""") + assert out == {"beforeClear": True, "afterClear": False} def test_background_done_sets_marker_when_session_not_actively_viewed(): @@ -550,17 +615,38 @@ def test_completion_unread_clears_only_when_session_is_opened(): assert "_setSessionViewedCount(sid, messageCount);" in ack_body -def test_historical_sessions_are_not_marked_unread_on_list_render(): - """The explicit unread marker must be event-driven, not initialized by _hasUnreadForSession.""" - has_unread_idx = SESSIONS_JS.find("function _hasUnreadForSession(s)") - assert has_unread_idx != -1 - has_unread_block = SESSIONS_JS[ - has_unread_idx:SESSIONS_JS.find("function _isSessionActivelyViewedForList", has_unread_idx) - ] +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_historical_sessions_are_not_marked_unread_on_first_list_render(): + """First observation establishes a read baseline instead of inventing unread.""" + out = _run_unread_behavior(""" +const unread = _hasUnreadForSession({ + session_id: 'X', message_count: 5, transcript_generation: 3, + transcript_generation_baseline: 0, +}); +const viewed = JSON.parse(store[SESSION_VIEWED_COUNTS_KEY]).X; +console.log(JSON.stringify({unread, viewed})); +""") + assert out == { + "unread": False, + "viewed": {"message_count": 5, "transcript_generation": 3}, + } - assert "_markSessionCompletionUnread" not in has_unread_block, ( - "rendering old historical sessions must not create completion-unread markers" - ) - assert "_setSessionViewedCount(s.session_id, Number(s.message_count || 0), generation);" in has_unread_block, ( - "missing viewed-count baseline should still initialize as read for historical sessions" - ) + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_new_transcript_generation_keeps_post_shrink_messages_unread(): + """A coalesced shrink and growth acknowledges only the retained baseline.""" + out = _run_unread_behavior(""" +store[SESSION_VIEWED_COUNTS_KEY] = JSON.stringify({ + X: {message_count: 10, transcript_generation: 0}, +}); +const unread = _hasUnreadForSession({ + session_id: 'X', message_count: 3, transcript_generation: 1, + transcript_generation_baseline: 2, +}); +const viewed = JSON.parse(store[SESSION_VIEWED_COUNTS_KEY]).X; +console.log(JSON.stringify({unread, viewed})); +""") + assert out == { + "unread": True, + "viewed": {"message_count": 2, "transcript_generation": 1}, + } diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index ab7b38c4..d09adc9d 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -125,6 +125,7 @@ _HELPER_FNS = [ "_nextSessionCompletionUnreadOrder", "_sessionCompletionUnreadClearedKey", "_parseSessionCompletionUnreadClearedKey", + "_readPersistedSessionCompletionUnreadCleared", "_readSessionCompletionUnreadCleared", "_mergeSessionCompletionUnread", "_writeSessionCompletionUnreadCleared", @@ -327,6 +328,36 @@ console.log(JSON.stringify({ assert out == {"before": True, "after": False, "diskHasX": False} +def test_quota_failed_clear_keeps_the_older_durable_tombstone_after_reload(): + """An in-memory-only higher clear must not prune the durable ordering fact.""" + out = _run_node(_script(""" +listed('X'); +const oldClearKey = `${SESSION_COMPLETION_UNREAD_CLEARED_PREFIX}${encodeURIComponent('X')}:2000`; +_store[oldClearKey] = '1000'; +const A = makeClient(); +const ordinarySetItem = localStorage.setItem; +localStorage.setItem = (key, value) => { + if (!Object.prototype.hasOwnProperty.call(_store, key)) throw new Error('QuotaExceededError'); + ordinarySetItem(key, value); +}; +// This advances X to 2001 in A's memory, but quota rejects that new durable key. +A.clearUnread('X'); +const durableAfterClear = Object.prototype.hasOwnProperty.call(_store, oldClearKey); +// Simulate a reload, then a stale client restoring a marker the durable clear +// already outranks. Module memory is gone; only localStorage may suppress it. +const B = makeClient(); +setDisk(SESSION_COMPLETION_UNREAD_KEY, { + X: {message_count: 3, completed_at: 1500, unread_order: 1500}, +}); +B.handleStorage({ key: oldClearKey }); +console.log(JSON.stringify({ + durableAfterClear, + unreadAfterReload: B.hasUnread('X'), +})); +""")) + assert out == {"durableAfterClear": True, "unreadAfterReload": False} + + def test_stale_client_cannot_roll_a_viewed_count_back(): """Sequential rollback: a client that saw an older list snapshot must not lower a count another client already acknowledged.""" diff --git a/tests/test_webui_run_state_consistency_contract_rfc.py b/tests/test_webui_run_state_consistency_contract_rfc.py index dedee864..b8c38a10 100644 --- a/tests/test_webui_run_state_consistency_contract_rfc.py +++ b/tests/test_webui_run_state_consistency_contract_rfc.py @@ -46,15 +46,6 @@ def test_contract_documents_the_client_side_unread_store_keys(): ) -def test_contract_documents_generation_scoped_viewed_counts(): - text = RFC.read_text(encoding="utf-8") - source = _sessions_js() - - assert "transcript_generation" in source - assert "transcript_generation" in text - assert "monotonic only within" in text - - def test_documented_retention_window_matches_the_implemented_cap(): source = _sessions_js() match = re.search(