From ef31e3ac430337ea451baf4c000f1fe5dea8f977 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 05:31:29 +0300 Subject: [PATCH 01/16] fix(sessions): stop cross-client clobber of unread state Two WebUI clients on the same origin/profile share one localStorage but each cache the unread stores in module state and re-serialise that cache wholesale. With no storage listener for either key, a stale client overwrites the other's writes: a session's viewed count can roll backwards (re-lighting a cleared unread dot via message_count > viewed_count) and a cleared completion-unread marker can be resurrected. Fix the class: - max-merge viewed counts on save so a count is monotonic and additive across clients, never dropping another client's entries or lowering a value; - keep the in-cache viewed count monotonic on set; - delete a viewed count directly from the persisted map (removals must still leave disk, unlike the additive set path); - invalidate both unread caches on storage events so a concurrent write is re-read instead of overwritten. Adds tests/test_unread_cross_client_sync.py (node-driven, observable behavior): the viewed-count rollback fails before the fix and passes after; the clear path keeps pruning; the storage-event handler prevents marker resurrection. --- static/sessions.js | 52 +++++++- tests/test_unread_cross_client_sync.py | 176 +++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 tests/test_unread_cross_client_sync.py diff --git a/static/sessions.js b/static/sessions.js index 76c1fdc9..b0289aba 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -478,7 +478,27 @@ function _knownSessionProfileCount(profile) { function _saveSessionViewedCounts() { try { - localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(_getSessionViewedCounts())); + // Merge against the persisted map instead of blindly overwriting it. Another + // WebUI client on the same origin/profile may have written newer counts since + // this client's cache was last loaded; serialising only our (possibly stale) + // in-memory copy would drop those entries and could roll a session's viewed + // count backwards, resurrecting a cleared unread dot. Max-merging keeps the + // map monotonic and additive across clients. + const cache = _getSessionViewedCounts(); + let merged = {}; + try { + const parsed = JSON.parse(localStorage.getItem(SESSION_VIEWED_COUNTS_KEY) || '{}'); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) merged = parsed; + } catch (_){ + merged = {}; + } + for (const sid of Object.keys(cache)) { + const prev = Number(merged[sid]) || 0; + const next = Number(cache[sid]) || 0; + merged[sid] = Math.max(prev, next); + } + localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(merged)); + _sessionViewedCounts = merged; } catch (_){ // Ignore localStorage write failures. } @@ -488,7 +508,10 @@ function _setSessionViewedCount(sid, messageCount = 0) { if (!sid) return; const counts = _getSessionViewedCounts(); const next = Number.isFinite(messageCount) ? Number(messageCount) : 0; - counts[sid] = next; + // A viewed count records "seen at least up to N messages" — keep it monotonic. + // A transient lower count (e.g. a list snapshot mid-compaction) must not + // resurrect a cleared unread dot before the save below. + counts[sid] = Math.max(Number(counts[sid]) || 0, next); _saveSessionViewedCounts(); // If the viewed count is now current, any prior completion-unread marker is // stale — clear it so _hasUnreadForSession doesn't short-circuit (#3020). @@ -689,6 +712,20 @@ function _clearSessionViewedCount(sid) { const counts = _getSessionViewedCounts(); if (!Object.prototype.hasOwnProperty.call(counts, sid)) return; delete counts[sid]; + // A removal must actually leave disk, unlike the additive max-merge used on + // the set path (which never deletes). Delete just this key from the persisted + // map so entries a concurrent client added are preserved. + try { + const parsed = JSON.parse(localStorage.getItem(SESSION_VIEWED_COUNTS_KEY) || '{}'); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + delete parsed[sid]; + localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(parsed)); + _sessionViewedCounts = parsed; + return; + } + } catch (_){ + // Fall through to the cache-only write on read/parse failure. + } _saveSessionViewedCounts(); } @@ -9055,10 +9092,21 @@ async function _handleShowAllProfilesStorageEvent(e){ if(typeof renderSessionList==='function') await renderSessionList({deferWhileInteracting:false}); } +function _handleUnreadStorageEvent(e){ + if(!e || !e.key) return; + // A concurrent WebUI client on the same origin/profile changed one of the + // unread stores. Drop our cached copy so the next read re-syncs from + // localStorage instead of our stale in-memory map clobbering their write + // (which would resurrect a cleared completion-unread marker). + if(e.key === SESSION_VIEWED_COUNTS_KEY) _sessionViewedCounts = null; + else if(e.key === SESSION_COMPLETION_UNREAD_KEY) _sessionCompletionUnread = null; +} + if(typeof window!=='undefined'){ window.addEventListener('storage', (e) => { void _handleActiveSessionStorageEvent(e); void _handleShowAllProfilesStorageEvent(e); + void _handleUnreadStorageEvent(e); }); window.addEventListener('popstate', () => { const sid=(typeof _sessionIdFromLocation==='function')?_sessionIdFromLocation():null; diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py new file mode 100644 index 00000000..a81cfdf5 --- /dev/null +++ b/tests/test_unread_cross_client_sync.py @@ -0,0 +1,176 @@ +"""Regression: unread state must not be clobbered by a second WebUI client. + +Two WebUI windows/tabs on the same origin and browser profile share one +``localStorage``. Each client caches the unread stores +(``hermes-session-viewed-counts`` and ``hermes-session-completion-unread``) in +module state and serialises that cache wholesale on save. With no ``storage`` +listener for either key, a stale client overwrites the other client's writes: + +- a session's *viewed* count can be rolled backwards, so ``message_count > + viewed_count`` re-lights a dot the user just cleared; and +- a completion-unread marker cleared in one client is resurrected by the other. + +This fixes the class by (a) max-merging viewed counts on save so a count can +never go down or drop another client's entries, and (b) invalidating both +caches on ``storage`` events so a concurrent write is re-read instead of +overwritten. +""" +import json +import subprocess +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") + + +def _extract(name: str) -> str: + """Extract a top-level `function name(...) { ... }` definition by brace match.""" + marker = f"function {name}(" + start = SESSIONS_JS.index(marker) + brace = SESSIONS_JS.index("{", start) + depth = 0 + for i in range(brace, len(SESSIONS_JS)): + ch = SESSIONS_JS[i] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + return SESSIONS_JS[start:i + 1] + raise AssertionError(f"could not brace-match {name}") + + +def _run_node(script: str) -> dict: + result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + return json.loads(result.stdout) + + +_VIEWED_FNS = [ + "_getSessionViewedCounts", + "_saveSessionViewedCounts", + "_setSessionViewedCount", + "_clearSessionViewedCount", +] +_UNREAD_FNS = [ + "_getSessionCompletionUnread", + "_saveSessionCompletionUnread", + "_markSessionCompletionUnread", + "_clearSessionCompletionUnread", + "_hasSessionCompletionUnread", +] + +_HEADER = """ +const _store = {}; +const localStorage = { + getItem: (k) => (k in _store ? _store[k] : null), + setItem: (k, v) => { _store[k] = String(v); }, +}; +const SESSION_VIEWED_COUNTS_KEY = 'hermes-session-viewed-counts'; +const SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'; +""" + + +def _make_client_block(): + """A factory that defines one client's module state and returns its API.""" + fns = "\n".join(_extract(n) for n in _VIEWED_FNS + _UNREAD_FNS) + handler = "" + if "function _handleUnreadStorageEvent(" in SESSIONS_JS: + handler = _extract("_handleUnreadStorageEvent") + return f""" +function makeClient() {{ + let _sessionViewedCounts = null; + let _sessionCompletionUnread = null; + {fns} + {handler} + return {{ + setViewed: _setSessionViewedCount, + clearViewed: _clearSessionViewedCount, + getViewed: (sid) => {{ + const c = _getSessionViewedCounts(); + return c[sid] === undefined ? null : c[sid]; + }}, + markUnread: _markSessionCompletionUnread, + clearUnread: _clearSessionCompletionUnread, + hasUnread: _hasSessionCompletionUnread, + handleStorage: (typeof _handleUnreadStorageEvent === 'function') ? _handleUnreadStorageEvent : null, + }}; +}} +""" + + +def test_viewed_count_is_monotonic_across_clients(): + """A stale client must not roll a session's viewed count backwards. + + Fails before the fix: client B serialises its stale cache and overwrites + client A's newer value, so the cleared dot re-lights via the count rule. + """ + script = _HEADER + _make_client_block() + """ +const A = makeClient(); +const B = makeClient(); +A.setViewed('X', 417); +B.setViewed('X', 131); // stale client believes the session has 131 msgs +console.log(JSON.stringify({ + disk: _store['hermes-session-viewed-counts'] ? JSON.parse(_store['hermes-session-viewed-counts']).X : null, + bCache: B.getViewed('X'), +})); +""" + out = _run_node(script) + assert out["disk"] == 417, ( + "a stale client must not lower a session's viewed count (disk was " + f"{out['disk']}, expected 417)" + ) + assert out["bCache"] == 417, ( + "the stale client's cache must be re-synced to the merged count" + ) + + +def test_clear_viewed_count_still_removes_from_disk(): + """Guard: session deletion must still prune the viewed-count entry.""" + script = _HEADER + _make_client_block() + """ +const A = makeClient(); +A.setViewed('D', 5); +A.clearViewed('D'); +const parsed = JSON.parse(_store['hermes-session-viewed-counts'] || '{}'); +console.log(JSON.stringify({ diskStillHas: Object.prototype.hasOwnProperty.call(parsed, 'D') })); +""" + out = _run_node(script) + assert out["diskStillHas"] is False, "clearing a viewed count must remove it from disk" + + +def test_storage_event_invalidates_unread_caches(): + """A storage event for either unread key must drop the in-memory cache so a + concurrent client's write is re-read, not clobbered. + + Skipped before the fix (the handler does not exist yet); passes after. + """ + if "function _handleUnreadStorageEvent(" not in SESSIONS_JS: + import pytest + pytest.skip("_handleUnreadStorageEvent not present yet") + + script = _HEADER + _make_client_block() + """ +const A = makeClient(); +const B = makeClient(); +A.markUnread('Y', 3); // A records a completion +B.hasUnread('Y'); // B loads the marker into its cache +A.clearUnread('Y'); // A clears it (e.g. the user visited Y in A) +// B sees A's storage event and must forget its cached copy. +B.handleStorage({ key: 'hermes-session-completion-unread' }); +B.markUnread('Z', 7); // B writes for an unrelated session +const disk = JSON.parse(_store['hermes-session-completion-unread'] || '{}'); +console.log(JSON.stringify({ + yResurrected: Object.prototype.hasOwnProperty.call(disk, 'Y'), + zPresent: Object.prototype.hasOwnProperty.call(disk, 'Z'), +})); +""" + out = _run_node(script) + assert out["zPresent"] is True, "B's new marker must be persisted" + assert out["yResurrected"] is False, ( + "a marker cleared in one client must not be resurrected by another" + ) + + +def test_unread_storage_handler_is_wired(): + """The window storage listener must route to the unread-cache handler.""" + assert "void _handleUnreadStorageEvent(e);" in SESSIONS_JS, ( + "the storage listener must invoke _handleUnreadStorageEvent" + ) From 9ffdaba4d96227b359c4c59226824e637579b105 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 13:11:36 +0300 Subject: [PATCH 02/16] fix(sessions): converge the unread stores across WebUI clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two WebUI clients on the same origin/profile share one localStorage, and each caches the unread stores in module state. Max-merging viewed counts on save and dropping the caches on a storage event was not enough: a save read the store, merged its cache and wrote the result back, so a write landing inside another client's read-modify-write window was still lost — and the surviving client only forgot its own cache, so it re-read the value that had already been lost. A viewed count could roll backwards and re-light a cleared dot. The completion markers behind the visible dot were worse: that save was a wholesale overwrite with no merge at all, so a stale client re-serialising its cache re-added a marker another client had cleared. Make both stores converge on the later fact rather than on the last writer: - viewed counts mean "seen at least up to N messages", so they merge with a max: monotonic per session and additive across clients. A value that exists only in our cache is written back only for a session the session list still shows (_allSessions / _sessionListSnapshotById), so a session deleted in another client, already pruned from the store by _clearSessionViewedCount, is not resurrected. - completion markers are add/remove and cannot be ordered by a max, so every clear records a tombstone (hermes-session-completion-unread-cleared, sid -> clearedAt) and a marker that does not postdate its clear loses. A genuine later completion still wins. The tombstone store never enters the marker map that consumers read, and is pruned by session existence and by a 7-day cap. Both clear paths record it: _clearSessionCompletionUnread (visit / delete) and _clearCronSessionCompletionUnreadForInactiveProfiles (profile switch). - both saves write only when the store does not already hold the merged result, and the storage listener runs that same merge instead of dropping our cache. A client that still holds an acknowledgement therefore re-asserts it after the other client's write, the loser sees a value it cannot beat and stops, and a repair converges instead of ping-ponging storage events. tests/test_unread_cross_client_sync.py drives the real functions against a two-client harness sharing one fake localStorage: an interleaved write inside a read-modify-write window, a deleted session's count, a stale client's marker write after a clear, tombstone ordering vs a later re-mark, tombstone pruning, and the registered storage listener dispatched with synthetic events. tests/_unread_store_helpers.py gives the neighbouring node harnesses the helpers those persistence paths call, so they keep driving the real code. --- static/sessions.js | 226 +++++++- tests/_unread_store_helpers.py | 77 +++ .../test_issue5273_active_done_viewed_sync.py | 8 + ...est_issue5960_cron_unread_profile_scope.py | 20 + tests/test_session_unread_dot_on_visit.py | 9 + tests/test_unread_cross_client_sync.py | 503 +++++++++++++++--- 6 files changed, 729 insertions(+), 114 deletions(-) create mode 100644 tests/_unread_store_helpers.py diff --git a/static/sessions.js b/static/sessions.js index b0289aba..4915ebfb 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -347,6 +347,11 @@ function _clearComposerDraft(sid, text, files) { const SESSION_VIEWED_COUNTS_KEY = 'hermes-session-viewed-counts'; const SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'; +// Cleared completion-unread markers leave a tombstone (sid -> clearedAt epoch ms) +// so a stale client that still holds the marker cannot resurrect it. Ordering +// facts live here, never in the marker map consumers read. +const SESSION_COMPLETION_UNREAD_CLEARED_KEY = 'hermes-session-completion-unread-cleared'; +const SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS = 7 * 24 * 60 * 60 * 1000; const SESSION_OBSERVED_STREAMING_KEY = 'hermes-session-observed-streaming'; // Per-profile session-count cache (issue #4717 / #4662 Phase 1.5). Records how // many sessions each profile rendered last time, keyed by profile name, so a @@ -476,29 +481,95 @@ function _knownSessionProfileCount(profile) { return (typeof v === 'number' && Number.isFinite(v)) ? v : null; } +// Read a persisted sid -> value map. Missing or unreadable values read as an +// empty map, matching the getters above. +function _readStoredJsonMap(key) { + try { + const parsed = JSON.parse(localStorage.getItem(key) || '{}'); + return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed : {}; + } catch (_){ + return {}; + } +} + +// Does this session still exist as far as this client can tell? The sidebar list +// is the authority (deletion drops the row) and the visit snapshot covers the +// window before the first list render. +function _sessionExistsForUnreadState(sid) { + if (!sid) return false; + if (typeof _allSessions !== 'undefined' && Array.isArray(_allSessions) + && _allSessions.some((s) => s && s.session_id === sid)) { + return true; + } + return typeof _sessionListSnapshotById !== 'undefined' && _sessionListSnapshotById + && typeof _sessionListSnapshotById.has === 'function' + && _sessionListSnapshotById.has(sid); +} + +// Is there a session list to judge existence against at all? Without one, +// "unknown" must not be read as "deleted": a client that has not rendered a list +// yet would otherwise refuse to persist a fresh acknowledgement, and would prune +// every tombstone it holds. +function _sessionListLoaded() { + if (typeof _allSessions !== 'undefined' && Array.isArray(_allSessions) + && _allSessions.length > 0) { + return true; + } + return typeof _sessionListSnapshotById !== 'undefined' && _sessionListSnapshotById + && typeof _sessionListSnapshotById.size === 'number' + && _sessionListSnapshotById.size > 0; +} + +// A viewed count is "seen at least up to N messages": monotonic per session and +// additive across clients, so merging is a max that never rolls a count back and +// never drops another client's entry. A value that exists only in our cache is +// written back only for a session the list still shows (see +// _sessionExistsForUnreadState) so a session deleted in another client, and +// already pruned from disk by _clearSessionViewedCount, is not resurrected. It +// is always kept in the live view regardless: dropping it here would lose the +// acknowledgement for a session that still exists. +function _mergeSessionViewedCounts(disk, cache) { + const counts = {}; + for (const sid of Object.keys(disk || {})) counts[sid] = Number(disk[sid]) || 0; + let changed = false; + for (const sid of Object.keys(cache || {})) { + const ours = Number(cache[sid]) || 0; + if (Object.prototype.hasOwnProperty.call(counts, sid)) { + if (ours > counts[sid]) { + counts[sid] = ours; + changed = true; + } + continue; + } + if (!ours) continue; + if (_sessionListLoaded() && !_sessionExistsForUnreadState(sid)) continue; + counts[sid] = ours; + changed = true; + } + const live = {}; + for (const sid of Object.keys(counts)) live[sid] = counts[sid]; + for (const sid of Object.keys(cache || {})) { + const ours = Number(cache[sid]) || 0; + if (ours > (Object.prototype.hasOwnProperty.call(live, sid) ? live[sid] : 0)) { + live[sid] = ours; + } + } + return {counts, live, changed}; +} + function _saveSessionViewedCounts() { try { - // Merge against the persisted map instead of blindly overwriting it. Another - // WebUI client on the same origin/profile may have written newer counts since - // this client's cache was last loaded; serialising only our (possibly stale) - // in-memory copy would drop those entries and could roll a session's viewed - // count backwards, resurrecting a cleared unread dot. Max-merging keeps the - // map monotonic and additive across clients. - const cache = _getSessionViewedCounts(); - let merged = {}; - try { - const parsed = JSON.parse(localStorage.getItem(SESSION_VIEWED_COUNTS_KEY) || '{}'); - if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) merged = parsed; - } catch (_){ - merged = {}; + const merged = _mergeSessionViewedCounts( + _readStoredJsonMap(SESSION_VIEWED_COUNTS_KEY), + _getSessionViewedCounts() + ); + // Write only when we hold something the store does not (a strictly greater + // count, or an entry it is missing). Rewriting an unchanged map would let two + // clients ping-pong storage events forever. + if (merged.changed) { + localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(merged.counts)); } - for (const sid of Object.keys(cache)) { - const prev = Number(merged[sid]) || 0; - const next = Number(cache[sid]) || 0; - merged[sid] = Math.max(prev, next); - } - localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(merged)); - _sessionViewedCounts = merged; + _sessionViewedCounts = merged.live; } catch (_){ // Ignore localStorage write failures. } @@ -529,9 +600,60 @@ function _getSessionCompletionUnread() { return _sessionCompletionUnread; } +// Completion-unread markers are add/remove, so unlike viewed counts they cannot +// be max-merged: the later event has to win. Every clear records a tombstone +// (sid -> clearedAt) in SESSION_COMPLETION_UNREAD_CLEARED_KEY, and a marker only +// survives when it does not predate its sid's clear. +function _markerLosesToUnreadClear(marker, clearedAt) { + const cleared = Number(clearedAt) || 0; + if (!cleared) return false; + return (Number(marker && marker.completed_at) || 0) <= cleared; +} + +// Merge the persisted marker map with our cache under the tombstones. Markers +// the store holds that a clear already outranks are dropped from the result, so +// the caller can write the repaired map back and no reader sees them again. +function _mergeSessionCompletionUnread(disk, cache, cleared) { + const markers = {}; + let changed = false; + for (const sid of Object.keys(disk || {})) { + const stored = disk[sid]; + if (_markerLosesToUnreadClear(stored, (cleared || {})[sid])) { + changed = true; + continue; + } + markers[sid] = stored; + } + for (const sid of Object.keys(cache || {})) { + const ours = cache[sid]; + if (_markerLosesToUnreadClear(ours, (cleared || {})[sid])) continue; + if (Object.prototype.hasOwnProperty.call(markers, sid)) { + const theirs = Number((markers[sid] || {}).completed_at) || 0; + if ((Number(ours && ours.completed_at) || 0) > theirs) { + markers[sid] = ours; + changed = true; + } + continue; + } + markers[sid] = ours; + changed = true; + } + return {markers, changed}; +} + function _saveSessionCompletionUnread() { try { - localStorage.setItem(SESSION_COMPLETION_UNREAD_KEY, JSON.stringify(_getSessionCompletionUnread())); + const merged = _mergeSessionCompletionUnread( + _readStoredJsonMap(SESSION_COMPLETION_UNREAD_KEY), + _getSessionCompletionUnread(), + _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY) + ); + // Same convergence rule as the viewed counts: write only when the store does + // not already hold the merged result. + if (merged.changed) { + localStorage.setItem(SESSION_COMPLETION_UNREAD_KEY, JSON.stringify(merged.markers)); + } + _sessionCompletionUnread = merged.markers; } catch (_){ // Ignore localStorage write failures. } @@ -573,11 +695,51 @@ function _markSessionCompletionUnreadIfBackground(sid, messageCount = null, meta return true; } +// Record that the completion-unread marker(s) for `sidOrSids` were cleared at +// `clearedAt`. This is the ordering fact a merge needs: markers carry +// completed_at, so a marker that does not postdate the clear loses to it, while a +// completion that happened afterwards still wins. The tombstone store is bounded +// like the maps it guards: entries for sessions the list no longer shows are +// dropped, as are entries older than SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS. +function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { + const sids = Array.isArray(sidOrSids) ? sidOrSids : [sidOrSids]; + const cleared = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); + for (const sid of sids) cleared[sid] = clearedAt; + const justCleared = new Set(sids); + const listLoaded = _sessionListLoaded(); + for (const key of Object.keys(cleared)) { + if (clearedAt - (Number(cleared[key]) || 0) > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS) { + delete cleared[key]; + continue; + } + // The tombstones just written stay even when their sessions are already + // gone: session deletion is exactly the case where the marker must not come + // back from a client that still holds it. + if (listLoaded && !justCleared.has(key) && !_sessionExistsForUnreadState(key)) { + delete cleared[key]; + } + } + try { + localStorage.setItem(SESSION_COMPLETION_UNREAD_CLEARED_KEY, JSON.stringify(cleared)); + } catch (_){ + // Ignore localStorage write failures; the marker map write below still runs. + } +} + function _clearSessionCompletionUnread(sid) { if (!sid) return; const unread = _getSessionCompletionUnread(); - if (!Object.prototype.hasOwnProperty.call(unread, sid)) return; + if (!Object.prototype.hasOwnProperty.call(unread, sid)) { + // Another client may have recorded this marker since our cache was loaded, + // so the store decides whether there is anything to clear. + const stored = Object.prototype.hasOwnProperty.call( + _readStoredJsonMap(SESSION_COMPLETION_UNREAD_KEY), sid); + if (!stored) return; + } delete unread[sid]; + // Durable ordering fact, written before the marker map: a client that still + // holds this marker (completed_at not after the clear) loses to it. + _writeSessionCompletionUnreadCleared(sid, Date.now()); _saveSessionCompletionUnread(); } @@ -689,6 +851,7 @@ function _clearCronSessionCompletionUnreadForInactiveProfiles(activeProfile) { : 'default'; const unread = _getSessionCompletionUnread(); let changed = false; + const clearedSids = []; for (const sid of Object.keys(unread)) { const marker = unread[sid]; if (!marker || typeof marker !== 'object' || Array.isArray(marker)) continue; @@ -699,9 +862,14 @@ function _clearCronSessionCompletionUnreadForInactiveProfiles(activeProfile) { if (!resolved.profile) continue; if (_cronMarkerProfileMatchesActive(resolved.profile, active)) continue; delete unread[sid]; + clearedSids.push(sid); changed = true; } if (!changed) return false; + // Each cleared marker needs its durable ordering fact, or the merge in + // _saveSessionCompletionUnread() (and a stale client's later save) would + // restore it from the store. + _writeSessionCompletionUnreadCleared(clearedSids, Date.now()); _saveSessionCompletionUnread(); if (typeof renderSessionListFromCache === 'function') renderSessionListFromCache(); return true; @@ -9095,11 +9263,15 @@ async function _handleShowAllProfilesStorageEvent(e){ function _handleUnreadStorageEvent(e){ if(!e || !e.key) return; // A concurrent WebUI client on the same origin/profile changed one of the - // unread stores. Drop our cached copy so the next read re-syncs from - // localStorage instead of our stale in-memory map clobbering their write - // (which would resurrect a cleared completion-unread marker). - if(e.key === SESSION_VIEWED_COUNTS_KEY) _sessionViewedCounts = null; - else if(e.key === SESSION_COMPLETION_UNREAD_KEY) _sessionCompletionUnread = null; + // unread stores. Both saves re-read the store, merge our cache into it (max for + // viewed counts, tombstone-ordered for completion markers), keep the merged map + // as the live cache, and write back only what the store is missing. So this + // client re-asserts the acknowledgement it still holds instead of dropping it + // and re-reading a value the other client's write already lost. + if(e.key === SESSION_VIEWED_COUNTS_KEY) _saveSessionViewedCounts(); + else if(e.key === SESSION_COMPLETION_UNREAD_KEY || e.key === SESSION_COMPLETION_UNREAD_CLEARED_KEY){ + _saveSessionCompletionUnread(); + } } if(typeof window!=='undefined'){ diff --git a/tests/_unread_store_helpers.py b/tests/_unread_store_helpers.py new file mode 100644 index 00000000..abe2e1ea --- /dev/null +++ b/tests/_unread_store_helpers.py @@ -0,0 +1,77 @@ +"""Node-harness scaffolding shared by the unread-store persistence tests. + +``static/sessions.js`` persists two unread stores (``hermes-session-viewed-counts`` +and ``hermes-session-completion-unread``) through a small set of pure helpers: +reading a stored map, asking whether a session still exists, and merging a cache +into a store under the monotonic / tombstone rules. The node harnesses in this +suite work by extracting individual functions from the module by name, so a +harness that drives a persistence path must also provide those helpers. + +Keeping the list here means every harness injects the same set from the same +source, and the store keys / tombstone cap always match the module. +""" +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") + +_HELPER_NAMES = ( + "_readStoredJsonMap", + "_sessionExistsForUnreadState", + "_sessionListLoaded", + "_mergeSessionViewedCounts", + "_markerLosesToUnreadClear", + "_mergeSessionCompletionUnread", + "_writeSessionCompletionUnreadCleared", +) + +# Declared from the module source so a harness cannot drift from the values the +# module uses. Harnesses keep their own stub keys for the two older stores. +_CONST_NAMES = ( + "SESSION_COMPLETION_UNREAD_CLEARED_KEY", + "SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS", +) + +_CONST_FALLBACKS = { + "SESSION_COMPLETION_UNREAD_CLEARED_KEY": ( + "const SESSION_COMPLETION_UNREAD_CLEARED_KEY = " + "'hermes-session-completion-unread-cleared';" + ), + "SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS": ( + "const SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS = 7 * 24 * 60 * 60 * 1000;" + ), +} + + +def _function_source(name: str) -> str: + """Brace-match a top-level ``function name(...) { ... }`` definition.""" + marker = f"function {name}(" + if marker not in SESSIONS_JS: + return "" + start = SESSIONS_JS.index(marker) + brace = SESSIONS_JS.index("{", start) + depth = 0 + for i in range(brace, len(SESSIONS_JS)): + ch = SESSIONS_JS[i] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + return SESSIONS_JS[start:i + 1] + raise AssertionError(f"could not brace-match {name}") + + +def _const_source(name: str) -> str: + for line in SESSIONS_JS.splitlines(): + stripped = line.strip() + if stripped.startswith(f"const {name} ="): + return stripped + return _CONST_FALLBACKS.get(name, "") + + +# Ready to interpolate into a node harness. +BLOCK = "\n".join( + [_const_source(name) for name in _CONST_NAMES] + + [_function_source(name) for name in _HELPER_NAMES if f"function {name}(" in SESSIONS_JS] +) diff --git a/tests/test_issue5273_active_done_viewed_sync.py b/tests/test_issue5273_active_done_viewed_sync.py index 26fada8c..da5d8594 100644 --- a/tests/test_issue5273_active_done_viewed_sync.py +++ b/tests/test_issue5273_active_done_viewed_sync.py @@ -1,6 +1,7 @@ import json import shutil import subprocess +import sys import textwrap from pathlib import Path @@ -11,6 +12,11 @@ ROOT = Path(__file__).resolve().parent.parent MESSAGES_JS = (ROOT / "static" / "messages.js").read_text(encoding="utf-8") SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") +# Sibling harness scaffolding lives beside this file and is not on sys.path by +# default; import it the same way the other node-harness tests do. +sys.path.insert(0, str(ROOT / "tests")) +import _unread_store_helpers as unread_store_helpers # noqa: E402 + def _function_body(src: str, name: str) -> str: marker = f"function {name}" @@ -91,6 +97,8 @@ def _run_done_compaction_harness( _extract_function(SESSIONS_JS, "_hasSessionCompletionUnread"), _extract_function(SESSIONS_JS, "_hasUnreadForSession"), _extract_function(SESSIONS_JS, "_markSessionCompletedInList"), + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, ] ) has_pre_list_sync = _has_pre_list_view_sync() diff --git a/tests/test_issue5960_cron_unread_profile_scope.py b/tests/test_issue5960_cron_unread_profile_scope.py index f98b4d76..8c7b5c66 100644 --- a/tests/test_issue5960_cron_unread_profile_scope.py +++ b/tests/test_issue5960_cron_unread_profile_scope.py @@ -5,6 +5,7 @@ from __future__ import annotations import json import shutil import subprocess +import sys from pathlib import Path import pytest @@ -16,6 +17,11 @@ SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") ROUTES_PY = (ROOT / "api" / "routes.py").read_text(encoding="utf-8") NODE = shutil.which("node") +# Sibling harness scaffolding lives beside this file and is not on sys.path by +# default; import it the same way the other node-harness tests do. +sys.path.insert(0, str(ROOT / "tests")) +import _unread_store_helpers as unread_store_helpers # noqa: E402 + def _extract_function(source: str, name: str) -> str: start = source.index(f"function {name}(") @@ -152,6 +158,8 @@ def test_profile_switch_clears_persisted_old_profile_cron_markers_only(): save_unread = _extract_function(SESSIONS_JS, "_saveSessionCompletionUnread") clear_helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(SESSIONS_JS, "_isCronSessionForUnread"), _extract_function(SESSIONS_JS, "_sourceKeyForSession"), _extract_function(SESSIONS_JS, "_cronCompletionUnreadMetaForSession"), @@ -371,6 +379,8 @@ def test_legacy_untagged_cron_marker_cleared_via_sidebar_metadata(): """Re-gate #5975: untagged markers resolve from sidebar session source/profile.""" helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(SESSIONS_JS, "_isCronSessionForUnread"), _extract_function(SESSIONS_JS, "_sourceKeyForSession"), _extract_function(SESSIONS_JS, "_cronCompletionUnreadMetaForSession"), @@ -434,6 +444,8 @@ def test_root_alias_keeps_current_profile_cron_marker(): """Re-gate #5975: default/renamed-root must not erase current-root cron dots.""" helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(SESSIONS_JS, "_isCronSessionForUnread"), _extract_function(SESSIONS_JS, "_sourceKeyForSession"), _extract_function(SESSIONS_JS, "_cronCompletionUnreadMetaForSession"), @@ -495,6 +507,8 @@ def test_switch_to_literal_default_clears_other_profile_cron_markers(): """ helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(SESSIONS_JS, "_isCronSessionForUnread"), _extract_function(SESSIONS_JS, "_sourceKeyForSession"), _extract_function(SESSIONS_JS, "_cronCompletionUnreadMetaForSession"), @@ -575,6 +589,8 @@ def test_switch_to_literal_default_without_roster_fails_closed_on_unknown_names( gets exact-name semantics (cleared), and literal-'default' markers stay.""" helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(SESSIONS_JS, "_isCronSessionForUnread"), _extract_function(SESSIONS_JS, "_sourceKeyForSession"), _extract_function(SESSIONS_JS, "_cronCompletionUnreadMetaForSession"), @@ -636,6 +652,8 @@ def test_stale_pre_switch_session_list_does_not_recreate_cron_markers(): mark_poll = _extract_function(sessions_js, "_markPollingCompletionUnreadTransitions") helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(sessions_js, "_isCronSessionForUnread"), _extract_function(sessions_js, "_sourceKeyForSession"), _extract_function(sessions_js, "_cronCompletionUnreadMetaForSession"), @@ -765,6 +783,8 @@ def test_fresh_session_list_still_marks_when_unread_gen_matches(): mark_poll = _extract_function(sessions_js, "_markPollingCompletionUnreadTransitions") helpers = "\n".join( [ + # Merge/tombstone helpers the unread persistence paths call. + unread_store_helpers.BLOCK, _extract_function(sessions_js, "_isCronSessionForUnread"), _extract_function(sessions_js, "_sourceKeyForSession"), _extract_function(sessions_js, "_cronCompletionUnreadMetaForSession"), diff --git a/tests/test_session_unread_dot_on_visit.py b/tests/test_session_unread_dot_on_visit.py index df358d3f..6865922a 100644 --- a/tests/test_session_unread_dot_on_visit.py +++ b/tests/test_session_unread_dot_on_visit.py @@ -29,11 +29,17 @@ Two invariants flagged in review are protected here and MUST NOT regress: """ import json import subprocess +import sys from pathlib import Path ROOT = Path(__file__).resolve().parents[1] SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") +# Sibling harness scaffolding lives beside this file and is not on sys.path by +# default; import it the same way the other node-harness tests do. +sys.path.insert(0, str(ROOT / "tests")) +import _unread_store_helpers as unread_store_helpers # noqa: E402 + def _load_session_block() -> str: start = SESSIONS_JS.index("async function loadSession(sid") @@ -195,6 +201,7 @@ function _forgetObservedStreamingSession() {{}} {set_viewed} {sync} {ack} +{unread_store_helpers.BLOCK} // Seed a stale completion-unread marker for the open session. _getSessionCompletionUnread()['open'] = {{message_count: 5, completed_at: 1}}; _saveSessionCompletionUnread(); @@ -272,6 +279,7 @@ const _sessionListSourceById = new Map(); {effective} {sync} {ack} +{unread_store_helpers.BLOCK} function _isSessionActivelyViewedForList(sid) {{ if (!sid || !S.session || S.session.session_id !== sid) return false; if (_loadingSessionId && _loadingSessionId !== sid) return false; @@ -380,6 +388,7 @@ async function api(url) {{ _apiCalled = true; return _apiResult; }} {set_viewed} {actively_viewed} {ensure} +{unread_store_helpers.BLOCK} function _hasMarker() {{ return Object.prototype.hasOwnProperty.call(_getSessionCompletionUnread(), 'open'); diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index a81cfdf5..35ddc829 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -1,26 +1,44 @@ """Regression: unread state must not be clobbered by a second WebUI client. Two WebUI windows/tabs on the same origin and browser profile share one -``localStorage``. Each client caches the unread stores -(``hermes-session-viewed-counts`` and ``hermes-session-completion-unread``) in -module state and serialises that cache wholesale on save. With no ``storage`` -listener for either key, a stale client overwrites the other client's writes: +``localStorage``, and each caches the unread stores in module state. A stale +client that re-serialises its cache wholesale -- or that drops its cache and +re-reads a value another client's write already destroyed -- breaks two stores: -- a session's *viewed* count can be rolled backwards, so ``message_count > - viewed_count`` re-lights a dot the user just cleared; and -- a completion-unread marker cleared in one client is resurrected by the other. +- ``hermes-session-viewed-counts``: a viewed count means "seen at least up to N + messages", so it is monotonic per session and additive across clients. It must + never roll back, and after a concurrent write the client that holds the higher + count must re-assert it into the store (repair) instead of dropping its cache + and re-reading whatever the losing write left behind. -This fixes the class by (a) max-merging viewed counts on save so a count can -never go down or drop another client's entries, and (b) invalidating both -caches on ``storage`` events so a concurrent write is re-read instead of -overwritten. +- ``hermes-session-completion-unread``: markers are add/remove, so they cannot be + max-merged. Every clear records a tombstone in + ``hermes-session-completion-unread-cleared`` (sid -> clearedAt), and the later + event wins: a marker whose ``completed_at`` does not postdate the clear is + dropped, while a genuinely later re-mark survives. This is the store behind the + visible dot, and the reported field symptom was a cleared dot coming back. + +These tests drive the real functions extracted from ``static/sessions.js`` +against a two-client harness that shares one fake ``localStorage``. The harness +injects the merge/tombstone helpers when the module defines them, so the same +assertions run against the pre-fix code and report the observable failure. """ import json +import shutil import subprocess from pathlib import Path +import pytest + ROOT = Path(__file__).resolve().parents[1] SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + +VIEWED_KEY = "hermes-session-viewed-counts" +UNREAD_KEY = "hermes-session-completion-unread" +CLEARED_KEY = "hermes-session-completion-unread-cleared" def _extract(name: str) -> str: @@ -40,18 +58,47 @@ def _extract(name: str) -> str: raise AssertionError(f"could not brace-match {name}") +def _extract_optional(name: str) -> str: + """Extraction for functions a change may add (empty string when absent). + + The same assertions therefore run before and after the fix: on the pre-fix + code the harness simply has no merge/tombstone helper to inject. + """ + return _extract(name) if f"function {name}(" in SESSIONS_JS else "" + + +def _extract_storage_listener() -> str: + """The module-level ``window.addEventListener('storage', ...)`` statement.""" + marker = "window.addEventListener('storage'" + start = SESSIONS_JS.index(marker) + brace = SESSIONS_JS.index("{", start) + depth = 0 + for i in range(brace, len(SESSIONS_JS)): + ch = SESSIONS_JS[i] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + return SESSIONS_JS[start:i + 3] # include the closing "});" + raise AssertionError("could not brace-match the storage listener") + + def _run_node(script: str) -> dict: - result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + 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) -_VIEWED_FNS = [ +# Functions the unread stores are made of. All of these exist before and after +# the fix, so a missing one is a real extraction failure. +_CLIENT_FNS = [ "_getSessionViewedCounts", "_saveSessionViewedCounts", "_setSessionViewedCount", "_clearSessionViewedCount", -] -_UNREAD_FNS = [ "_getSessionCompletionUnread", "_saveSessionCompletionUnread", "_markSessionCompletionUnread", @@ -59,118 +106,400 @@ _UNREAD_FNS = [ "_hasSessionCompletionUnread", ] -_HEADER = """ -const _store = {}; -const localStorage = { - getItem: (k) => (k in _store ? _store[k] : null), - setItem: (k, v) => { _store[k] = String(v); }, -}; -const SESSION_VIEWED_COUNTS_KEY = 'hermes-session-viewed-counts'; -const SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'; +# Helpers that carry the merge/tombstone rules; injected when present. +_HELPER_FNS = [ + "_readStoredJsonMap", + "_sessionExistsForUnreadState", + "_sessionListLoaded", + "_mergeSessionViewedCounts", + "_markerLosesToUnreadClear", + "_mergeSessionCompletionUnread", + "_writeSessionCompletionUnreadCleared", +] + +_HEADER = f""" +// One fake localStorage shared by every client, counting writes so a test can +// assert a repair converges instead of ping-ponging writes between clients. +const _store = {{}}; +let storageWrites = 0; +let _onRead = null; +const localStorage = {{ + getItem: (k) => {{ + const value = Object.prototype.hasOwnProperty.call(_store, k) ? _store[k] : null; + // Test hook: runs after the value is captured, i.e. inside a read-modify-write. + if (_onRead) {{ const hook = _onRead; _onRead = null; hook(); }} + return value; + }}, + setItem: (k, v) => {{ storageWrites += 1; _store[k] = String(v); }}, + removeItem: (k) => {{ delete _store[k]; }}, +}}; +const SESSION_VIEWED_COUNTS_KEY = {VIEWED_KEY!r}; +const SESSION_COMPLETION_UNREAD_KEY = {UNREAD_KEY!r}; +const SESSION_COMPLETION_UNREAD_CLEARED_KEY = {CLEARED_KEY!r}; +// Mirrors the module constant. The age assertions use hours vs 8 days so they +// do not depend on the exact cap. +const SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS = 7 * 24 * 60 * 60 * 1000; +let _now = 1000; +Date.now = () => _now; +// The session list this client can see: the authority on "still exists". +let _allSessions = []; +const _sessionListSnapshotById = new Map(); +const listed = (...ids) => {{ _allSessions = ids.map((sid) => ({{session_id: sid}})); }}; +const readDisk = (key) => JSON.parse(_store[key] || '{{}}'); +const setDisk = (key, value) => {{ _store[key] = JSON.stringify(value); }}; +const _storageHandlers = []; +const window = {{ + addEventListener: (type, fn) => {{ + if (type === 'storage') _storageHandlers.push(fn); + }}, +}}; +// The neighbours the storage registration also routes to; only the unread +// handler is under test here. +async function _handleActiveSessionStorageEvent() {{}} +async function _handleShowAllProfilesStorageEvent() {{}} """ -def _make_client_block(): - """A factory that defines one client's module state and returns its API.""" - fns = "\n".join(_extract(n) for n in _VIEWED_FNS + _UNREAD_FNS) - handler = "" - if "function _handleUnreadStorageEvent(" in SESSIONS_JS: - handler = _extract("_handleUnreadStorageEvent") +def _client_factory(with_listener: bool = False) -> str: + """One client's module state plus the whole unread-store API.""" + fns = "\n".join(_extract(name) for name in _CLIENT_FNS) + helpers = "\n".join(_extract_optional(name) for name in _HELPER_FNS) + handler = _extract_optional("_handleUnreadStorageEvent") + listener = _extract_storage_listener() if with_listener else "" return f""" function makeClient() {{ let _sessionViewedCounts = null; let _sessionCompletionUnread = null; {fns} + {helpers} {handler} + {listener} return {{ + viewed: (sid) => {{ + const counts = _getSessionViewedCounts(); + return Object.prototype.hasOwnProperty.call(counts, sid) ? counts[sid] : null; + }}, setViewed: _setSessionViewedCount, clearViewed: _clearSessionViewedCount, - getViewed: (sid) => {{ - const c = _getSessionViewedCounts(); - return c[sid] === undefined ? null : c[sid]; - }}, markUnread: _markSessionCompletionUnread, clearUnread: _clearSessionCompletionUnread, hasUnread: _hasSessionCompletionUnread, - handleStorage: (typeof _handleUnreadStorageEvent === 'function') ? _handleUnreadStorageEvent : null, + handleStorage: (typeof _handleUnreadStorageEvent === 'function') + ? _handleUnreadStorageEvent + : () => {{}}, }}; }} """ -def test_viewed_count_is_monotonic_across_clients(): - """A stale client must not roll a session's viewed count backwards. +def _script(body: str, with_listener: bool = False) -> str: + return _HEADER + _client_factory(with_listener) + body - Fails before the fix: client B serialises its stale cache and overwrites - client A's newer value, so the cleared dot re-lights via the count rule. - """ - script = _HEADER + _make_client_block() + """ + +# ── Viewed counts: monotonic, repaired, never resurrected ──────────────────── + +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.""" + out = _run_node(_script(""" +listed('X'); const A = makeClient(); const B = makeClient(); A.setViewed('X', 417); -B.setViewed('X', 131); // stale client believes the session has 131 msgs +B.setViewed('X', 131); console.log(JSON.stringify({ - disk: _store['hermes-session-viewed-counts'] ? JSON.parse(_store['hermes-session-viewed-counts']).X : null, - bCache: B.getViewed('X'), + disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + bView: B.viewed('X'), })); -""" - out = _run_node(script) - assert out["disk"] == 417, ( - "a stale client must not lower a session's viewed count (disk was " - f"{out['disk']}, expected 417)" - ) - assert out["bCache"] == 417, ( - "the stale client's cache must be re-synced to the merged count" +""")) + assert out["disk"] == {"X": 417}, ( + "a stale client must not lower a session's viewed count" ) + assert out["bView"] == 417, "the stale client must converge on the merged count" def test_clear_viewed_count_still_removes_from_disk(): - """Guard: session deletion must still prune the viewed-count entry.""" - script = _HEADER + _make_client_block() + """ + """Guard: session deletion must still prune the viewed count.""" + out = _run_node(_script(""" +listed('D'); const A = makeClient(); A.setViewed('D', 5); A.clearViewed('D'); -const parsed = JSON.parse(_store['hermes-session-viewed-counts'] || '{}'); -console.log(JSON.stringify({ diskStillHas: Object.prototype.hasOwnProperty.call(parsed, 'D') })); -""" - out = _run_node(script) - assert out["diskStillHas"] is False, "clearing a viewed count must remove it from disk" +console.log(JSON.stringify({ + diskHasD: Object.prototype.hasOwnProperty.call(readDisk(SESSION_VIEWED_COUNTS_KEY), 'D'), +})); +""")) + assert out["diskHasD"] is False, "clearing a viewed count must remove it from disk" -def test_storage_event_invalidates_unread_caches(): - """A storage event for either unread key must drop the in-memory cache so a - concurrent client's write is re-read, not clobbered. - - Skipped before the fix (the handler does not exist yet); passes after. - """ - if "function _handleUnreadStorageEvent(" not in SESSIONS_JS: - import pytest - pytest.skip("_handleUnreadStorageEvent not present yet") - - script = _HEADER + _make_client_block() + """ +def test_interleaved_acknowledgements_both_survive(): + """Both clients read one baseline, then each write lands inside the other's + read-modify-write window. Neither acknowledgement may be lost.""" + out = _run_node(_script(""" +listed('X', 'Y'); const A = makeClient(); const B = makeClient(); -A.markUnread('Y', 3); // A records a completion -B.hasUnread('Y'); // B loads the marker into its cache -A.clearUnread('Y'); // A clears it (e.g. the user visited Y in A) -// B sees A's storage event and must forget its cached copy. -B.handleStorage({ key: 'hermes-session-completion-unread' }); -B.markUnread('Z', 7); // B writes for an unrelated session -const disk = JSON.parse(_store['hermes-session-completion-unread'] || '{}'); +// Both clients read the same baseline before either write commits. +A.viewed('X'); +B.viewed('Y'); +// A's save reads the store; B's write for its own session lands in that window, +// so A's write (built from the store it read) drops B's entry. +_onRead = () => { B.setViewed('Y', 999); }; +A.setViewed('X', 417); +const clobbered = readDisk(SESSION_VIEWED_COUNTS_KEY); +// B still holds its own acknowledgement, and now processes A's storage event. +B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); +const disk = readDisk(SESSION_VIEWED_COUNTS_KEY); +const writesAfterRepair = storageWrites; +B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); console.log(JSON.stringify({ - yResurrected: Object.prototype.hasOwnProperty.call(disk, 'Y'), - zPresent: Object.prototype.hasOwnProperty.call(disk, 'Z'), + clobbered: clobbered, + disk: disk, + stable: storageWrites === writesAfterRepair, + aView: A.viewed('X'), + bView: B.viewed('Y'), })); -""" - out = _run_node(script) - assert out["zPresent"] is True, "B's new marker must be persisted" - assert out["yResurrected"] is False, ( - "a marker cleared in one client must not be resurrected by another" +""")) + assert out["clobbered"] == {"X": 417}, ( + "precondition: the interleaved write dropped B's entry" + ) + assert out["disk"] == {"X": 417, "Y": 999}, ( + "the client that still holds an acknowledgement must re-assert it after " + "the other client's storage event, not re-read the store it just lost" + ) + assert out["aView"] == 417 and out["bView"] == 999, ( + "both clients must converge on a view that contains both acknowledgements" + ) + assert out["stable"] is True, ( + "a repair must converge: a second event must not write again" ) -def test_unread_storage_handler_is_wired(): - """The window storage listener must route to the unread-cache handler.""" - assert "void _handleUnreadStorageEvent(e);" in SESSIONS_JS, ( - "the storage listener must invoke _handleUnreadStorageEvent" +def test_repair_does_not_resurrect_a_deleted_sessions_viewed_count(): + """A session deleted in one client is pruned from the store; a stale client + that still caches its count must not write it back.""" + out = _run_node(_script(""" +listed('S', 'T'); +const A = makeClient(); +const B = makeClient(); +A.setViewed('S', 5); +B.viewed('S'); +A.clearViewed('S'); +listed('T'); +B.setViewed('T', 1); +B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); +console.log(JSON.stringify({ + disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + tView: B.viewed('T'), +})); +""")) + assert out["disk"] == {"T": 1}, ( + "a deleted session's pruned count must not be resurrected by a stale " + f"client's save or repair (disk was {out['disk']})" + ) + assert out["tView"] == 1, "the live acknowledgement for the listed session must survive" + + +# ── Completion unread: the later event wins ────────────────────────────────── + +def test_stale_client_cannot_resurrect_a_cleared_completion_marker(): + """The field symptom: a marker cleared by opening the chat must not come + back when another client that still holds it saves later.""" + out = _run_node(_script(""" +listed('Y', 'Z'); +const A = makeClient(); +const B = makeClient(); +_now = 1000; +A.markUnread('Y', 3); +B.hasUnread('Y'); +_now = 2000; +A.clearUnread('Y'); +// B has not processed A's storage event yet and saves for another session. +B.markUnread('Z', 7); +const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); +console.log(JSON.stringify({ + y: Object.prototype.hasOwnProperty.call(disk, 'Y'), + z: Object.prototype.hasOwnProperty.call(disk, 'Z'), + yViewA: A.hasUnread('Y'), +})); +""")) + assert out["z"] is True, "precondition: the stale client's own marker is persisted" + assert out["y"] is False, ( + "a marker cleared in one client must not be resurrected by another " + "client that still holds it" + ) + assert out["yViewA"] is False, "the cleared marker must stay cleared for the clear's own client" + + +def test_a_genuine_later_re_mark_survives_the_earlier_clear(): + """Control: ordering must not over-clear. A completion that happened after + the clear is new information and must win.""" + out = _run_node(_script(""" +listed('Y'); +const A = makeClient(); +const B = makeClient(); +_now = 1000; +A.markUnread('Y', 3); +B.hasUnread('Y'); +_now = 2000; +A.clearUnread('Y'); +_now = 3000; +A.markUnread('Y', 9); +B.handleStorage({ key: SESSION_COMPLETION_UNREAD_KEY }); +const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); +console.log(JSON.stringify({ + marker: disk.Y || null, + aView: A.hasUnread('Y'), + bView: B.hasUnread('Y'), +})); +""")) + assert out["marker"] == {"message_count": 9, "completed_at": 3000}, ( + "a completion that postdates the clear must be kept" + ) + assert out["aView"] is True and out["bView"] is True, ( + "both clients must show the genuine re-mark" + ) + + +def test_repair_removes_a_marker_that_lost_to_a_clear(): + """A stale client's write can land after the clear. The repair must drop the + losing marker from the store, or the dot returns on every client.""" + out = _run_node(_script(""" +listed('Y', 'Z'); +const A = makeClient(); +const B = makeClient(); +_now = 1000; +A.markUnread('Y', 3); +B.hasUnread('Y'); +_now = 2000; +A.clearUnread('Y'); +// The stale write lands after the clear (saved before the event was processed). +setDisk(SESSION_COMPLETION_UNREAD_KEY, { + Y: { message_count: 3, completed_at: 1000 }, + Z: { message_count: 2, completed_at: 1500 }, +}); +A.handleStorage({ key: SESSION_COMPLETION_UNREAD_KEY }); +const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); +console.log(JSON.stringify({ + diskY: Object.prototype.hasOwnProperty.call(disk, 'Y'), + diskZ: Object.prototype.hasOwnProperty.call(disk, 'Z'), + yView: A.hasUnread('Y'), + zView: A.hasUnread('Z'), +})); +""")) + assert out["diskY"] is False, ( + "a repair must remove a marker that lost to a clear, not leave it for " + "the next reader" + ) + assert out["diskZ"] is True, "an unrelated marker must be left alone" + assert out["yView"] is False and out["zView"] is True, ( + "the repaired view must match the store" + ) + + +def test_tombstones_are_pruned_when_their_session_is_gone(): + """Tombstones are keyed by session id; one whose session no longer exists + must not accumulate.""" + out = _run_node(_script(""" +listed('Y', 'GONE'); +const A = makeClient(); +_now = 1000; +A.markUnread('GONE', 1); +A.clearUnread('GONE'); +listed('Y'); +_now = 2000; +A.markUnread('Y', 1); +A.clearUnread('Y'); +console.log(JSON.stringify({ tombs: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY) })); +""")) + assert out["tombs"] == {"Y": 2000}, ( + "only the tombstone for a session that still exists may be kept" + ) + + +def test_tombstones_are_capped_by_age(): + """Quota guard: an old tombstone is dropped when a newer clear is recorded.""" + out = _run_node(_script(""" +listed('Y', 'Z'); +const A = makeClient(); +_now = 1000; +A.markUnread('Y', 1); +A.clearUnread('Y'); +const young = _now + 60 * 60 * 1000; +_now = young; +A.markUnread('Z', 1); +A.clearUnread('Z'); +const keptYoung = readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY); +_now = young + 8 * 24 * 60 * 60 * 1000; +A.markUnread('Y', 1); +A.clearUnread('Y'); +console.log(JSON.stringify({ + keptYoung: keptYoung, + afterAge: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), + young: young, +})); +""")) + assert out["keptYoung"] == {"Y": 1000, "Z": out["young"]}, ( + "a tombstone younger than the cap must survive a newer clear" + ) + assert out["afterAge"] == {"Y": out["young"] + 8 * 24 * 60 * 60 * 1000}, ( + "a tombstone older than the cap must be dropped" + ) + + +# ── The registration itself ────────────────────────────────────────────────── + +def test_registered_storage_listener_syncs_both_unread_keys_from_storage(): + """Behavioural wiring test: capture the handler the module registers on + ``storage``, dispatch synthetic events for both keys, and assert the client's + view comes from the updated store.""" + out = _run_node(_script(""" +listed('X', 'Q', 'R'); +const A = makeClient(); +const dispatch = (key) => { + const value = Object.prototype.hasOwnProperty.call(_store, key) ? _store[key] : null; + for (const handler of _storageHandlers) handler({ key: key, newValue: value }); +}; +A.setViewed('X', 511); +A.hasUnread('Q'); +// A stale client clobbers the store with its lower count. +setDisk(SESSION_VIEWED_COUNTS_KEY, { X: 131 }); +dispatch(SESSION_VIEWED_COUNTS_KEY); +const writesAfterRepair = storageWrites; +dispatch(SESSION_VIEWED_COUNTS_KEY); +const repairStable = storageWrites === writesAfterRepair; +// A marker another client just recorded, and a clear it recorded. +setDisk(SESSION_COMPLETION_UNREAD_KEY, { Q: { message_count: 2, completed_at: 5000 } }); +const qStale = A.hasUnread('Q'); +dispatch(SESSION_COMPLETION_UNREAD_KEY); +setDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY, { R: 9000 }); +setDisk(SESSION_COMPLETION_UNREAD_KEY, { + Q: { message_count: 2, completed_at: 5000 }, + R: { message_count: 3, completed_at: 8000 }, +}); +dispatch(SESSION_COMPLETION_UNREAD_CLEARED_KEY); +const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); +console.log(JSON.stringify({ + handlers: _storageHandlers.length, + xView: A.viewed('X'), + diskX: readDisk(SESSION_VIEWED_COUNTS_KEY).X, + stable: repairStable, + qStale: qStale, + qView: A.hasUnread('Q'), + rView: A.hasUnread('R'), + diskR: Object.prototype.hasOwnProperty.call(disk, 'R'), +})); +""", with_listener=True)) + assert out["handlers"] == 1, "the module must register exactly one storage listener" + assert out["xView"] == 511 and out["diskX"] == 511, ( + "a storage event must repair the store with the count this client still " + "holds instead of adopting the clobbering value" + ) + assert out["stable"] is True, "the repair must not write again for the same state" + assert out["qStale"] is False and out["qView"] is True, ( + "a storage event for the unread key must refresh the client's marker view" + ) + assert out["rView"] is False and out["diskR"] is False, ( + "the cleared-marker key must be routed to the handler and its tombstone " + "must win over the stale marker in the store" ) From 000b85a4504ce67aaee57d3b7bbe77dc6d7b4353 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 15:18:36 +0300 Subject: [PATCH 03/16] fix(sessions): prune viewed counts only the store holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_clearSessionViewedCount` returned early when this client's cache did not hold the session id, so it never consulted the persisted map. A viewed count another client acknowledged after our cache was loaded therefore survived session deletion, leaving one entry behind per deleted session — the quota pressure this prune path exists to avoid — and a later repair merged that stale entry back into the store. Read the store on every deletion and drop the key when the store holds it. The deletion must not cost this client the acknowledgement it holds for another session, so adopt the store's entries while keeping any higher count already cached — the merge the repair path uses — instead of replacing the cache with the store map. The write stays conditional, so a deletion that changes nothing does not fire a storage event. Behavioural two-client coverage in tests/test_unread_cross_client_sync.py: `test_delete_prunes_a_viewed_count_only_the_store_holds` and `test_delete_keeps_the_higher_count_this_client_holds_for_others`. Both fail on the previous revision (the store keeps the deleted session's count, and the repair writes it back) and pass with this change. --- static/sessions.js | 19 ++++++--- tests/test_unread_cross_client_sync.py | 57 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/static/sessions.js b/static/sessions.js index 4915ebfb..96cc8179 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -878,23 +878,30 @@ function _clearCronSessionCompletionUnreadForInactiveProfiles(activeProfile) { function _clearSessionViewedCount(sid) { if (!sid) return; const counts = _getSessionViewedCounts(); - if (!Object.prototype.hasOwnProperty.call(counts, sid)) return; + const hadCachedEntry = Object.prototype.hasOwnProperty.call(counts, sid); delete counts[sid]; // A removal must actually leave disk, unlike the additive max-merge used on // the set path (which never deletes). Delete just this key from the persisted - // map so entries a concurrent client added are preserved. + // map so entries a concurrent client added are preserved. The store is + // consulted even when our cache never held the key: an entry another client + // added since our cache was loaded is exactly the one this path has to prune. try { const parsed = JSON.parse(localStorage.getItem(SESSION_VIEWED_COUNTS_KEY) || '{}'); if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - delete parsed[sid]; - localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(parsed)); - _sessionViewedCounts = parsed; + if (Object.prototype.hasOwnProperty.call(parsed, sid)) { + delete parsed[sid]; + localStorage.setItem(SESSION_VIEWED_COUNTS_KEY, JSON.stringify(parsed)); + } + // Adopt the store's entries but keep any higher count we already hold: we + // are the only client left that can repair a count another client + // lowered, so dropping ours here would lose the acknowledgement. + _sessionViewedCounts = _mergeSessionViewedCounts(parsed, counts).live; return; } } catch (_){ // Fall through to the cache-only write on read/parse failure. } - _saveSessionViewedCounts(); + if (hadCachedEntry) _saveSessionViewedCounts(); } function _hasSessionCompletionUnread(sid) { diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 35ddc829..e3e21b68 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -232,6 +232,63 @@ console.log(JSON.stringify({ assert out["diskHasD"] is False, "clearing a viewed count must remove it from disk" +def test_delete_prunes_a_viewed_count_only_the_store_holds(): + """Deletion must consult the store, not just our cache. An entry another + client added after this client's cache was loaded is exactly what the prune + path exists to remove, and an early return on the cache miss leaves it + behind for a session the list no longer shows.""" + out = _run_node(_script(""" +listed('E'); +const A = makeClient(); +const B = makeClient(); +// B's cache loads while the store is still empty, so B never sees E. +B.viewed('E'); +// A acknowledges E, which B cannot know about from its own cache. +A.setViewed('E', 17); +const bCached = B.viewed('E'); +B.clearViewed('E'); +console.log(JSON.stringify({ + bCached: bCached, + disk: readDisk(SESSION_VIEWED_COUNTS_KEY), +})); +""")) + assert out["bCached"] is None, "precondition: B's cache must not hold E" + assert out["disk"] == {}, ( + "deleting a session must prune its viewed count from the store even " + "when this client's cache never held the key" + ) + + +def test_delete_keeps_the_higher_count_this_client_holds_for_others(): + """Pruning one session must not cost this client the acknowledgement it + holds for another. Adopting the store's view wholesale while pruning would + drop a count another client already lowered, and this client is the only + one that can still repair it.""" + out = _run_node(_script(""" +listed('X', 'E'); +const A = makeClient(); +A.setViewed('X', 417); +// Another client lowered X while recording its own acknowledgement for E. +setDisk(SESSION_VIEWED_COUNTS_KEY, { X: 131, E: 17 }); +A.clearViewed('E'); +const held = A.viewed('X'); +// A now processes the storage event that clobber arrived with. +A.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); +console.log(JSON.stringify({ + held: held, + disk: readDisk(SESSION_VIEWED_COUNTS_KEY), +})); +""")) + assert out["held"] == 417, ( + "pruning one session must not drop the higher count this client holds " + "for another" + ) + assert out["disk"] == {"X": 417}, ( + "the client must still hold the acknowledgement that repairs the " + "lowered count in the store" + ) + + def test_interleaved_acknowledgements_both_survive(): """Both clients read one baseline, then each write lands inside the other's read-modify-write window. Neither acknowledgement may be lost.""" From 927f1ca4f72190016d60c16b78b8a0995d2e1e08 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 15:48:02 +0300 Subject: [PATCH 04/16] docs(rfc): document the client-side unread persistence contract The run state consistency contract is where sidebar metadata work routes, and its state-layer table already lists the sidebar layer, but nothing recorded the rules the two client-side unread stores actually follow: viewed counts converge by a max merge, completion markers are ordered by per-session tombstones with a bounded retention window, and the storage listener repairs the store instead of only dropping the local cache. Write those rules down where they belong, naming the three localStorage keys and the retention cap, and state the limitation explicitly: each save is a read-modify-write across separate localStorage operations with no lock between them, so the contract is convergence rather than atomicity, and two concurrent clears can still lose a tombstone. tests/test_webui_run_state_consistency_contract_rfc.py pins the documented keys and the documented retention window to the constants in static/sessions.js, so changing one without the other fails. It fails on the previous revision and passes with this change. --- .../webui-run-state-consistency-contract.md | 49 +++++++++++++- ...ebui_run_state_consistency_contract_rfc.py | 65 +++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 tests/test_webui_run_state_consistency_contract_rfc.py diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index ca185de6..f7fd90b9 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -3,7 +3,7 @@ - **Status:** Proposed - **Author:** @franksong2702 - **Created:** 2026-05-16 -- **Updated:** 2026-08-22 +- **Updated:** 2026-09-15 - **Tracking issue:** [#2361](https://github.com/nesquena/hermes-webui/issues/2361) - **Related architecture:** [#1925](https://github.com/nesquena/hermes-webui/issues/1925), [`hermes-run-adapter-contract.md`](hermes-run-adapter-contract.md), [`stable-assistant-turn-anchors.md`](stable-assistant-turn-anchors.md) @@ -75,6 +75,7 @@ and 5; it does not mark every run-state boundary implemented. | Compression summary / handoff | Gives the agent recovery context after automatic compression | Must remain agent-facing recovery material unless explicitly rendered as history | Pollute the active turn or become implicit current user intent | | Live UI scene/cache | Preserves expanded rows, in-progress cards, local scroll, and transient grouping | May optimize presentation but must be rebuildable or degradable from transcript/replay | Become the only place where chronological ordering exists | | Sidebar/session metadata | Helps the user find active and recent sessions | Must reflect meaningful user or assistant activity | Treat background cleanup as a fresh user-facing update | +| Client-side unread stores (`localStorage`) | Backs the sidebar unread dot for every client on the origin | Converges across clients by merge and tombstone ordering; not atomic | Let one client's stale cache lower a count or resurrect a cleared marker | ## Core Invariants @@ -153,6 +154,48 @@ and 5; it does not mark every run-state boundary implemented. timestamp (falling back to run start), so a long-running turn cancelled moments ago is never mistaken for an orphan. +## Client-side unread persistence (sidebar layer) + +The sidebar unread dot is backed by two client-side stores in `static/sessions.js`. +Both live in `localStorage` under the origin, so every WebUI client on the same +origin/profile (a PWA window and a browser tab, for example) shares them while each +client also caches them in module state. They are projections of the sidebar layer +above; the rules below describe what stays coherent when more than one client +writes. + +| Store | Key | Semantics | +|---|---|---| +| Viewed counts | `hermes-session-viewed-counts` | `sid -> N`, meaning "seen at least up to N messages" | +| Completion markers | `hermes-session-completion-unread` | `sid -> {message_count, completed_at, ...}` behind the visible dot | + +- **Viewed counts merge by maximum.** The fact is monotonic per session and + additive across clients, so a save reads the store, merges its cache in with a + max, and writes only when the store does not already hold the merged result. A + count held only in a client's cache is written back only for a session the + sidebar list still shows, so a session deleted in another client is not + resurrected. +- **Completion markers are ordered by tombstones.** Markers are add/remove and + cannot be max-ordered, so each clear records `sid -> clearedAt` in + `hermes-session-completion-unread-cleared`, and a marker that does not postdate + its clear loses; a genuine later completion still wins. The tombstone map is + bounded by session existence and a 7-day retention cap, and is never part of the + marker map consumers read. +- **Cross-client repair, not cache invalidation.** The `storage` listener routes a + changed unread key back through that same merge instead of only dropping the + local cache. A client that still holds an acknowledgement re-asserts it after the + other client's write, the loser sees a value it cannot beat and stops, and repair + converges instead of ping-ponging storage events. +- **The contract is convergence, not atomicity.** Each save is a read-modify-write + across separate `localStorage` operations with no lock between them, so two + clients writing inside the same window can still lose one update; repair + re-asserts held acknowledgements on the next storage event. The case repair + cannot fix is two concurrent *clears*: the later whole-map write can discard the + other client's tombstone, after which a later marker save can re-add the stale + marker and the cleared dot returns. A hard cross-tab guarantee needs either a + lock around those writes (Web Locks, with a documented fallback for contexts + without it) or per-session versioned records. This contract deliberately claims + only the convergence described here. + ## Review Checklist Use this checklist for PRs that touch run state, streaming, replay, compression, @@ -175,6 +218,10 @@ context reconstruction, or session metadata: - If it introduces or changes a reclamation window, what proves an in-flight cancellation is not evicted early, and that a wedged one is eventually freed? - Can automatic compression or recovery text become visible active-turn content? +- Does this change write one of the client-side unread stores + (`hermes-session-viewed-counts`, `hermes-session-completion-unread`, + `hermes-session-completion-unread-cleared`), and does it keep the merge and + tombstone rules in the client-side unread persistence section? - What test or manual evidence proves the invariant? ## Existing Issue Map diff --git a/tests/test_webui_run_state_consistency_contract_rfc.py b/tests/test_webui_run_state_consistency_contract_rfc.py new file mode 100644 index 00000000..b8c38a10 --- /dev/null +++ b/tests/test_webui_run_state_consistency_contract_rfc.py @@ -0,0 +1,65 @@ +"""The run state consistency contract must document the client-side unread +persistence rules, and what it names has to match what `static/sessions.js` +implements. + +The contract is prose, so it cannot be verified behaviourally. What rots silently +is the identifier set: the store keys the sidebar layer depends on and the +retention window that bounds the tombstone map. These assertions pin the +documented names to the implementation constants, so changing either one without +the other fails here instead of drifting. +""" + +import re +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +RFC = ROOT / "docs" / "rfcs" / "webui-run-state-consistency-contract.md" +SESSIONS_JS = ROOT / "static" / "sessions.js" + +# The stores the sidebar layer persists in localStorage, and the constant that +# bounds the tombstone map. +DOCUMENTED_CONSTANTS = [ + "SESSION_VIEWED_COUNTS_KEY", + "SESSION_COMPLETION_UNREAD_KEY", + "SESSION_COMPLETION_UNREAD_CLEARED_KEY", +] + + +def _sessions_js() -> str: + return SESSIONS_JS.read_text(encoding="utf-8") + + +def _string_constant(source: str, name: str) -> str: + match = re.search(rf"^const {name} = '([^']*)';", source, re.MULTILINE) + assert match, f"{name} must be defined as a string constant in static/sessions.js" + return match.group(1) + + +def test_contract_documents_the_client_side_unread_store_keys(): + text = RFC.read_text(encoding="utf-8") + source = _sessions_js() + + for name in DOCUMENTED_CONSTANTS: + key = _string_constant(source, name) + assert key in text, ( + f"the contract must name the store behind {name} ({key})" + ) + + +def test_documented_retention_window_matches_the_implemented_cap(): + source = _sessions_js() + match = re.search( + r"^const SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS = (\d+) \*", + source, + re.MULTILINE, + ) + assert match, ( + "SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS must be defined in days in " + "static/sessions.js" + ) + days = int(match.group(1)) + + text = RFC.read_text(encoding="utf-8") + assert f"{days}-day" in text, ( + f"the contract must state the {days}-day retention cap on the tombstone map" + ) From 9b81ac4d482ac05cb9e19f28e985c52835244902 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 16:53:29 +0300 Subject: [PATCH 05/16] fix: make unread clears race-safe across clients --- .../webui-run-state-consistency-contract.md | 47 +++--- static/sessions.js | 156 +++++++++++++----- tests/_unread_store_helpers.py | 4 + ...est_issue5960_cron_unread_profile_scope.py | 14 ++ tests/test_session_unread_dot_on_visit.py | 9 + tests/test_unread_cross_client_sync.py | 124 +++++++++++++- 6 files changed, 285 insertions(+), 69 deletions(-) diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index f7fd90b9..d15a4a20 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -75,7 +75,7 @@ and 5; it does not mark every run-state boundary implemented. | Compression summary / handoff | Gives the agent recovery context after automatic compression | Must remain agent-facing recovery material unless explicitly rendered as history | Pollute the active turn or become implicit current user intent | | Live UI scene/cache | Preserves expanded rows, in-progress cards, local scroll, and transient grouping | May optimize presentation but must be rebuildable or degradable from transcript/replay | Become the only place where chronological ordering exists | | Sidebar/session metadata | Helps the user find active and recent sessions | Must reflect meaningful user or assistant activity | Treat background cleanup as a fresh user-facing update | -| Client-side unread stores (`localStorage`) | Backs the sidebar unread dot for every client on the origin | Converges across clients by merge and tombstone ordering; not atomic | Let one client's stale cache lower a count or resurrect a cleared marker | +| Client-side unread stores (`localStorage`) | Backs the sidebar unread dot for every client on the origin | Converges counts/markers across clients and stores clear ordering independently per session | Let one client's stale cache lower a count or resurrect a cleared marker | ## Core Invariants @@ -174,27 +174,32 @@ writes. count held only in a client's cache is written back only for a session the sidebar list still shows, so a session deleted in another client is not resurrected. -- **Completion markers are ordered by tombstones.** Markers are add/remove and - cannot be max-ordered, so each clear records `sid -> clearedAt` in - `hermes-session-completion-unread-cleared`, and a marker that does not postdate - its clear loses; a genuine later completion still wins. The tombstone map is - bounded by session existence and a 7-day retention cap, and is never part of the - marker map consumers read. +- **Completion markers are ordered by per-session tombstones.** Markers are + add/remove and cannot be max-ordered, so each clear records `clearedAt` under + `hermes-session-completion-unread-cleared:v1::`. Independent + immutable version keys mean clients clearing different sessions—or clearing the + same session in an interleaved operation—cannot replace newer ordering facts. A + marker that does not postdate its clear loses; a genuine later + completion still wins. During rolling upgrades, the previous unsuffixed + `hermes-session-completion-unread-cleared` map is both read and dual-written so + already-open clients on the prior revision observe new clears; it remains a + temporary compatibility store and is not pruned in this revision. Versioned + tombstones are bounded by session existence and a 7-day retention cap, and are + never part of the marker map consumers read. - **Cross-client repair, not cache invalidation.** The `storage` listener routes a - changed unread key back through that same merge instead of only dropping the - local cache. A client that still holds an acknowledgement re-asserts it after the - other client's write, the loser sees a value it cannot beat and stops, and repair - converges instead of ping-ponging storage events. -- **The contract is convergence, not atomicity.** Each save is a read-modify-write - across separate `localStorage` operations with no lock between them, so two - clients writing inside the same window can still lose one update; repair - re-asserts held acknowledgements on the next storage event. The case repair - cannot fix is two concurrent *clears*: the later whole-map write can discard the - other client's tombstone, after which a later marker save can re-add the stale - marker and the cleared dot returns. A hard cross-tab guarantee needs either a - lock around those writes (Web Locks, with a documented fallback for contexts - without it) or per-session versioned records. This contract deliberately claims - only the convergence described here. + changed unread key (including any per-session clear key) back through the same + merge instead of only dropping the local cache. A client that still holds an + acknowledgement re-asserts it after the other client's write, the loser sees a + value it cannot beat and stops, and repair converges instead of ping-ponging + storage events. +- **Whole-map facts converge; clear ordering does not share a map.** Viewed counts + and completion markers still use read-modify-write maps, so interleaved writes + can transiently lose an entry. Their monotonic cache merge and storage-event + repair re-assert held facts. Clear ordering is different: each session has its + own atomic `localStorage` write, so concurrent clears of different sessions + cannot clobber each other. A viewed-count advance records its clear even when a + competing client has prepared but not yet persisted the older completion + marker; repeated observations at the same count do not refresh the tombstone. ## Review Checklist diff --git a/static/sessions.js b/static/sessions.js index 96cc8179..ef5f1492 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -347,10 +347,11 @@ function _clearComposerDraft(sid, text, files) { const SESSION_VIEWED_COUNTS_KEY = 'hermes-session-viewed-counts'; const SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'; -// Cleared completion-unread markers leave a tombstone (sid -> clearedAt epoch ms) -// so a stale client that still holds the marker cannot resurrect it. Ordering -// facts live here, never in the marker map consumers read. +// Clear ordering is stored per session so two clients clearing different +// sessions never replace one another's tombstones with whole-map writes. The +// unsuffixed key is the legacy map read during migration. const SESSION_COMPLETION_UNREAD_CLEARED_KEY = 'hermes-session-completion-unread-cleared'; +const SESSION_COMPLETION_UNREAD_CLEARED_PREFIX = `${SESSION_COMPLETION_UNREAD_CLEARED_KEY}:v1:`; const SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS = 7 * 24 * 60 * 60 * 1000; const SESSION_OBSERVED_STREAMING_KEY = 'hermes-session-observed-streaming'; // Per-profile session-count cache (issue #4717 / #4662 Phase 1.5). Records how @@ -579,14 +580,17 @@ function _setSessionViewedCount(sid, messageCount = 0) { if (!sid) return; const counts = _getSessionViewedCounts(); const next = Number.isFinite(messageCount) ? Number(messageCount) : 0; + const hadCount = Object.prototype.hasOwnProperty.call(counts, sid); + const previous = Number(counts[sid]) || 0; // A viewed count records "seen at least up to N messages" — keep it monotonic. // A transient lower count (e.g. a list snapshot mid-compaction) must not // resurrect a cleared unread dot before the save below. - counts[sid] = Math.max(Number(counts[sid]) || 0, next); + counts[sid] = Math.max(previous, next); _saveSessionViewedCounts(); - // If the viewed count is now current, any prior completion-unread marker is - // stale — clear it so _hasUnreadForSession doesn't short-circuit (#3020). - _clearSessionCompletionUnread(sid); + // Advancing the acknowledgement records a clear even if another client has + // only prepared (not persisted) its older completion marker. Repeated polling + // at the same count does not refresh the tombstone or emit storage events. + _clearSessionCompletionUnread(sid, !hadCount || next > previous); } function _getSessionCompletionUnread() { @@ -610,6 +614,42 @@ function _markerLosesToUnreadClear(marker, clearedAt) { return (Number(marker && marker.completed_at) || 0) <= cleared; } +function _sessionCompletionUnreadClearedKey(sid, clearedAt = null) { + const base = `${SESSION_COMPLETION_UNREAD_CLEARED_PREFIX}${encodeURIComponent(String(sid))}`; + return clearedAt == null ? base : `${base}:${Number(clearedAt) || 0}`; +} + +function _parseSessionCompletionUnreadClearedKey(key) { + if (!key || !key.startsWith(SESSION_COMPLETION_UNREAD_CLEARED_PREFIX)) return null; + const suffix = key.slice(SESSION_COMPLETION_UNREAD_CLEARED_PREFIX.length); + const separator = suffix.lastIndexOf(':'); + const encodedSid = separator > 0 ? suffix.slice(0, separator) : suffix; + const version = separator > 0 ? Number(suffix.slice(separator + 1)) || 0 : 0; + let sid = ''; + try { sid = decodeURIComponent(encodedSid); } catch (_) { return null; } + if (!sid) return null; + 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() { + const cleared = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); + try { + for (let i = 0; i < localStorage.length; i += 1) { + const key = localStorage.key(i); + const parsed = _parseSessionCompletionUnreadClearedKey(key); + if (!parsed) continue; + const value = Math.max(parsed.version, Number(localStorage.getItem(key)) || 0); + if (value > (Number(cleared[parsed.sid]) || 0)) cleared[parsed.sid] = value; + } + } catch (_) { + // A storage implementation that cannot enumerate still retains legacy data. + } + return cleared; +} + // Merge the persisted marker map with our cache under the tombstones. Markers // the store holds that a clear already outranks are dropped from the result, so // the caller can write the repaired map back and no reader sees them again. @@ -646,7 +686,7 @@ function _saveSessionCompletionUnread() { const merged = _mergeSessionCompletionUnread( _readStoredJsonMap(SESSION_COMPLETION_UNREAD_KEY), _getSessionCompletionUnread(), - _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY) + _readSessionCompletionUnreadCleared() ); // Same convergence rule as the viewed counts: write only when the store does // not already hold the merged result. @@ -663,7 +703,15 @@ function _markSessionCompletionUnread(sid, messageCount = 0, meta = null) { if (!sid) return; const unread = _getSessionCompletionUnread(); const count = Number.isFinite(messageCount) ? Number(messageCount) : 0; - const entry = {message_count: count, completed_at: Date.now()}; + // If a clear and a genuine later completion share one millisecond, preserve + // event order rather than letting the clear's <= comparison erase the later + // marker. + const intendedAt = Date.now(); + const clearedAt = Number(_readSessionCompletionUnreadCleared()[sid]) || 0; + const completedAt = clearedAt && clearedAt <= intendedAt + ? Math.max(intendedAt, clearedAt + 1) + : intendedAt; + const entry = {message_count: count, completed_at: completedAt}; // Cron markers carry source+profile so profile switches can clear only that // cross-profile leak without wiping ordinary chat completion unread (#5960). if (meta && typeof meta === 'object' && !Array.isArray(meta)) { @@ -696,49 +744,76 @@ function _markSessionCompletionUnreadIfBackground(sid, messageCount = null, meta } // Record that the completion-unread marker(s) for `sidOrSids` were cleared at -// `clearedAt`. This is the ordering fact a merge needs: markers carry -// completed_at, so a marker that does not postdate the clear loses to it, while a -// completion that happened afterwards still wins. The tombstone store is bounded -// like the maps it guards: entries for sessions the list no longer shows are -// dropped, as are entries older than SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS. +// `clearedAt`. Each session owns a separate key: clears for different sessions +// are independent atomic writes and cannot discard one another. Markers carry a +// completed_at timestamp, so a marker that does not postdate the clear loses, +// while a completion that happened afterwards still wins. Old or deleted-session +// tombstones are pruned opportunistically to bound quota use. function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { - const sids = Array.isArray(sidOrSids) ? sidOrSids : [sidOrSids]; - const cleared = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); - for (const sid of sids) cleared[sid] = clearedAt; - const justCleared = new Set(sids); - const listLoaded = _sessionListLoaded(); - for (const key of Object.keys(cleared)) { - if (clearedAt - (Number(cleared[key]) || 0) > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS) { - delete cleared[key]; - continue; - } - // The tombstones just written stay even when their sessions are already - // gone: session deletion is exactly the case where the marker must not come - // back from a client that still holds it. - if (listLoaded && !justCleared.has(key) && !_sessionExistsForUnreadState(key)) { - delete cleared[key]; - } + const justCleared = new Set( + (Array.isArray(sidOrSids) ? sidOrSids : [sidOrSids]).map((sid) => String(sid)) + ); + const at = Number(clearedAt) || 0; + const legacyClears = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); + for (const sid of justCleared) { + try { + localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, at), String(at)); + } catch (_) {} + // Rolling compatibility: clients already open on the previous revision only + // understand the whole-map key. Publish new clears there too, while the + // immutable key remains authoritative for clients on this revision. + legacyClears[sid] = Math.max(Number(legacyClears[sid]) || 0, at); } + try { localStorage.setItem(SESSION_COMPLETION_UNREAD_CLEARED_KEY, JSON.stringify(legacyClears)); } catch (_) {} + + // Copy ordering facts from the old whole-map representation, but keep the + // legacy map until this PR no longer needs rolling compatibility. An + // already-open older client only understands that key; deleting it would let + // that client reassert stale markers. Keeping it also avoids a racy + // read/copy/remove migration losing a clear written by an older client. + for (const [sid, value] of Object.entries(legacyClears)) { + const version = Number(value) || 0; + if (!version) continue; + try { + localStorage.setItem( + _sessionCompletionUnreadClearedKey(sid, version), String(version)); + } catch (_) {} + } + + const cleared = _readSessionCompletionUnreadCleared(); + const listLoaded = _sessionListLoaded(); + let keys = []; try { - localStorage.setItem(SESSION_COMPLETION_UNREAD_CLEARED_KEY, JSON.stringify(cleared)); - } catch (_){ - // Ignore localStorage write failures; the marker map write below still runs. + keys = Array.from({ length: localStorage.length }, (_, i) => localStorage.key(i)); + } catch (_) {} + for (const key of keys) { + const parsed = _parseSessionCompletionUnreadClearedKey(key); + if (!parsed) continue; + try { + const version = Math.max(parsed.version, Number(localStorage.getItem(key)) || 0); + const expired = at - version > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS; + const deleted = listLoaded && !justCleared.has(parsed.sid) + && !_sessionExistsForUnreadState(parsed.sid); + const superseded = version < (Number(cleared[parsed.sid]) || 0); + if (expired || deleted || superseded) localStorage.removeItem(key); + } catch (_) {} } } -function _clearSessionCompletionUnread(sid) { +function _clearSessionCompletionUnread(sid, recordIfMissing = true) { if (!sid) return; const unread = _getSessionCompletionUnread(); + let stored = false; if (!Object.prototype.hasOwnProperty.call(unread, sid)) { // Another client may have recorded this marker since our cache was loaded, // so the store decides whether there is anything to clear. - const stored = Object.prototype.hasOwnProperty.call( + stored = Object.prototype.hasOwnProperty.call( _readStoredJsonMap(SESSION_COMPLETION_UNREAD_KEY), sid); - if (!stored) return; + if (!stored && !recordIfMissing) return; } delete unread[sid]; - // Durable ordering fact, written before the marker map: a client that still - // holds this marker (completed_at not after the clear) loses to it. + // Write the ordering fact before the marker map. This is required even when + // the marker has been prepared by another client but has not reached storage. _writeSessionCompletionUnreadCleared(sid, Date.now()); _saveSessionCompletionUnread(); } @@ -9276,7 +9351,10 @@ function _handleUnreadStorageEvent(e){ // client re-asserts the acknowledgement it still holds instead of dropping it // and re-reading a value the other client's write already lost. if(e.key === SESSION_VIEWED_COUNTS_KEY) _saveSessionViewedCounts(); - else if(e.key === SESSION_COMPLETION_UNREAD_KEY || e.key === SESSION_COMPLETION_UNREAD_CLEARED_KEY){ + else if(e.key === SESSION_COMPLETION_UNREAD_KEY + || e.key === SESSION_COMPLETION_UNREAD_CLEARED_KEY + || (typeof e.key === 'string' + && e.key.startsWith(SESSION_COMPLETION_UNREAD_CLEARED_PREFIX))){ _saveSessionCompletionUnread(); } } diff --git a/tests/_unread_store_helpers.py b/tests/_unread_store_helpers.py index abe2e1ea..6837f69e 100644 --- a/tests/_unread_store_helpers.py +++ b/tests/_unread_store_helpers.py @@ -21,6 +21,9 @@ _HELPER_NAMES = ( "_sessionListLoaded", "_mergeSessionViewedCounts", "_markerLosesToUnreadClear", + "_sessionCompletionUnreadClearedKey", + "_parseSessionCompletionUnreadClearedKey", + "_readSessionCompletionUnreadCleared", "_mergeSessionCompletionUnread", "_writeSessionCompletionUnreadCleared", ) @@ -29,6 +32,7 @@ _HELPER_NAMES = ( # module uses. Harnesses keep their own stub keys for the two older stores. _CONST_NAMES = ( "SESSION_COMPLETION_UNREAD_CLEARED_KEY", + "SESSION_COMPLETION_UNREAD_CLEARED_PREFIX", "SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS", ) diff --git a/tests/test_issue5960_cron_unread_profile_scope.py b/tests/test_issue5960_cron_unread_profile_scope.py index 8c7b5c66..5dd51a94 100644 --- a/tests/test_issue5960_cron_unread_profile_scope.py +++ b/tests/test_issue5960_cron_unread_profile_scope.py @@ -181,6 +181,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; @@ -403,6 +405,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; @@ -468,6 +472,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; @@ -535,6 +541,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; @@ -614,6 +622,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; @@ -675,6 +685,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; @@ -803,6 +815,8 @@ global.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:(i)=>Object.keys(store)[i]??null, + get length(){{ return Object.keys(store).length; }}, }}; let _sessionCompletionUnread=null; let _sessionViewedCounts={{}}; diff --git a/tests/test_session_unread_dot_on_visit.py b/tests/test_session_unread_dot_on_visit.py index 6865922a..789062f9 100644 --- a/tests/test_session_unread_dot_on_visit.py +++ b/tests/test_session_unread_dot_on_visit.py @@ -182,6 +182,9 @@ const _store = {{}}; const localStorage = {{ getItem: (k) => (k in _store ? _store[k] : null), setItem: (k, v) => {{ _store[k] = String(v); }}, + removeItem: (k) => {{ delete _store[k]; }}, + key: (i) => Object.keys(_store)[i] ?? null, + get length() {{ return Object.keys(_store).length; }}, }}; const SESSION_VIEWED_COUNTS_KEY = 'v'; const SESSION_COMPLETION_UNREAD_KEY = 'u'; @@ -245,6 +248,9 @@ const _store = {{}}; const localStorage = {{ getItem: (k) => (k in _store ? _store[k] : null), setItem: (k, v) => {{ _store[k] = String(v); }}, + removeItem: (k) => {{ delete _store[k]; }}, + key: (i) => Object.keys(_store)[i] ?? null, + get length() {{ return Object.keys(_store).length; }}, }}; const SESSION_VIEWED_COUNTS_KEY = 'v'; const SESSION_COMPLETION_UNREAD_KEY = 'u'; @@ -343,6 +349,9 @@ const _store = {{}}; const localStorage = {{ getItem: (k) => (k in _store ? _store[k] : null), setItem: (k, v) => {{ _store[k] = String(v); }}, + removeItem: (k) => {{ delete _store[k]; }}, + key: (i) => Object.keys(_store)[i] ?? null, + get length() {{ return Object.keys(_store).length; }}, }}; const SESSION_VIEWED_COUNTS_KEY = 'v'; const SESSION_COMPLETION_UNREAD_KEY = 'u'; diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index e3e21b68..45077dd3 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -113,6 +113,9 @@ _HELPER_FNS = [ "_sessionListLoaded", "_mergeSessionViewedCounts", "_markerLosesToUnreadClear", + "_sessionCompletionUnreadClearedKey", + "_parseSessionCompletionUnreadClearedKey", + "_readSessionCompletionUnreadCleared", "_mergeSessionCompletionUnread", "_writeSessionCompletionUnreadCleared", ] @@ -132,10 +135,13 @@ const localStorage = {{ }}, setItem: (k, v) => {{ storageWrites += 1; _store[k] = String(v); }}, removeItem: (k) => {{ delete _store[k]; }}, + key: (i) => Object.keys(_store)[i] ?? null, + get length() {{ return Object.keys(_store).length; }}, }}; const SESSION_VIEWED_COUNTS_KEY = {VIEWED_KEY!r}; const SESSION_COMPLETION_UNREAD_KEY = {UNREAD_KEY!r}; const SESSION_COMPLETION_UNREAD_CLEARED_KEY = {CLEARED_KEY!r}; +const SESSION_COMPLETION_UNREAD_CLEARED_PREFIX = `${{SESSION_COMPLETION_UNREAD_CLEARED_KEY}}:v1:`; // Mirrors the module constant. The age assertions use hours vs 8 days so they // do not depend on the exact cap. const SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS = 7 * 24 * 60 * 60 * 1000; @@ -147,6 +153,10 @@ const _sessionListSnapshotById = new Map(); const listed = (...ids) => {{ _allSessions = ids.map((sid) => ({{session_id: sid}})); }}; const readDisk = (key) => JSON.parse(_store[key] || '{{}}'); const setDisk = (key, value) => {{ _store[key] = JSON.stringify(value); }}; +// Aggregate either the legacy whole map or independent per-session records. +const readAllClears = (client) => client && typeof client.clears === 'function' + ? client.clears() + : readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY); const _storageHandlers = []; const window = {{ addEventListener: (type, fn) => {{ @@ -183,7 +193,11 @@ function makeClient() {{ clearViewed: _clearSessionViewedCount, markUnread: _markSessionCompletionUnread, clearUnread: _clearSessionCompletionUnread, + writeClear: _writeSessionCompletionUnreadCleared, hasUnread: _hasSessionCompletionUnread, + clears: (typeof _readSessionCompletionUnreadCleared === 'function') + ? _readSessionCompletionUnreadCleared + : () => readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), handleStorage: (typeof _handleUnreadStorageEvent === 'function') ? _handleUnreadStorageEvent : () => {{}}, @@ -454,6 +468,92 @@ console.log(JSON.stringify({ ) +def test_concurrent_clears_keep_both_ordering_facts(): + """Clearing different sessions in the same read/write window must not let + either whole-map write erase the other session's ordering fact.""" + out = _run_node(_script(""" +listed('X', 'Y'); +const A = makeClient(); +const B = makeClient(); +_now = 1000; +A.markUnread('X', 1); +B.markUnread('Y', 1); +A.hasUnread('Y'); +B.hasUnread('X'); +_now = 2000; +// A captures the old tombstone state; B's clear lands before A writes. +_onRead = () => { B.clearUnread('Y'); }; +A.clearUnread('X'); +const tombs = readAllClears(A); +console.log(JSON.stringify({ tombs })); +""")) + assert out["tombs"] == {"X": 2000, "Y": 2000}, ( + "concurrent clears for different sessions must retain both tombstones" + ) + + +def test_new_clear_is_also_published_for_rolling_clients(): + """An already-open client on the previous PR head must see a clear created + by the new implementation in the legacy whole-map key.""" + out = _run_node(_script(""" +const A = makeClient(); +A.writeClear('new', 2000); +console.log(JSON.stringify({ legacy: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), all: readAllClears(A) })); +""")) + assert out["legacy"] == {"new": 2000} + assert out["all"] == {"new": 2000} + + +def test_legacy_tombstone_map_is_kept_for_rolling_clients(): + """An already-open client on the previous PR head only understands the + whole-map key, so migration must not delete it during a rolling session.""" + out = _run_node(_script(""" +setDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY, { legacy: 1000 }); +const A = makeClient(); +A.writeClear('new', 2000); +console.log(JSON.stringify({ legacy: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), all: readAllClears(A) })); +""")) + assert out["legacy"] == {"legacy": 1000, "new": 2000} + assert out["all"] == {"legacy": 1000, "new": 2000} + + +def test_same_session_clear_versions_fold_to_the_newest_operation(): + """An older clear operation that lands after a newer one must not lower the + ordering fact for that session.""" + out = _run_node(_script(""" +listed('Y'); +const A = makeClient(); +A.writeClear('Y', 2000); +A.writeClear('Y', 1000); +console.log(JSON.stringify({ tombs: readAllClears(A) })); +""")) + assert out["tombs"] == {"Y": 2000} + + +def test_clear_orders_out_a_marker_prepared_but_not_yet_persisted(): + """A user visit after completion intent but before its delayed marker write + must record a clear even when neither its cache nor storage has the marker.""" + out = _run_node(_script(""" +listed('Y'); +const A = makeClient(); +const B = makeClient(); +A.hasUnread('Y'); +B.hasUnread('Y'); +_now = 1000; +// B's marker carries t=1000. Its save captures the empty marker map, then A +// visits at t=2000 before B's delayed write commits. +_onRead = () => { _now = 2000; A.clearUnread('Y'); }; +B.markUnread('Y', 3); +const marker = readDisk(SESSION_COMPLETION_UNREAD_KEY).Y || null; +const tombstone = readAllClears(A).Y || null; +console.log(JSON.stringify({ marker, tombstone })); +""")) + assert out["tombstone"] == 2000, ( + "a clear must persist its ordering fact even when the marker write is delayed" + ) + assert out["marker"] is None, "the completion that predates the visit must remain cleared" + + def test_tombstones_are_pruned_when_their_session_is_gone(): """Tombstones are keyed by session id; one whose session no longer exists must not accumulate.""" @@ -467,10 +567,11 @@ listed('Y'); _now = 2000; A.markUnread('Y', 1); A.clearUnread('Y'); -console.log(JSON.stringify({ tombs: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY) })); +console.log(JSON.stringify({ tombs: readAllClears(A) })); """)) - assert out["tombs"] == {"Y": 2000}, ( - "only the tombstone for a session that still exists may be kept" + assert out["tombs"] == {"GONE": 1000, "Y": 2000}, ( + "the versioned tombstone may be pruned, while its legacy compatibility " + "fact remains available to already-open clients" ) @@ -486,21 +587,25 @@ const young = _now + 60 * 60 * 1000; _now = young; A.markUnread('Z', 1); A.clearUnread('Z'); -const keptYoung = readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY); +const keptYoung = readAllClears(A); _now = young + 8 * 24 * 60 * 60 * 1000; A.markUnread('Y', 1); A.clearUnread('Y'); console.log(JSON.stringify({ keptYoung: keptYoung, - afterAge: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), + afterAge: readAllClears(A), young: young, })); """)) assert out["keptYoung"] == {"Y": 1000, "Z": out["young"]}, ( "a tombstone younger than the cap must survive a newer clear" ) - assert out["afterAge"] == {"Y": out["young"] + 8 * 24 * 60 * 60 * 1000}, ( - "a tombstone older than the cap must be dropped" + assert out["afterAge"] == { + "Y": out["young"] + 8 * 24 * 60 * 60 * 1000, + "Z": out["young"], + }, ( + "the old versioned record is dropped, while the rollout compatibility " + "map remains available to older clients" ) @@ -529,12 +634,13 @@ const repairStable = storageWrites === writesAfterRepair; setDisk(SESSION_COMPLETION_UNREAD_KEY, { Q: { message_count: 2, completed_at: 5000 } }); const qStale = A.hasUnread('Q'); dispatch(SESSION_COMPLETION_UNREAD_KEY); -setDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY, { R: 9000 }); +const rClearKey = `${SESSION_COMPLETION_UNREAD_CLEARED_PREFIX}${encodeURIComponent('R')}:9000`; +_store[rClearKey] = '9000'; setDisk(SESSION_COMPLETION_UNREAD_KEY, { Q: { message_count: 2, completed_at: 5000 }, R: { message_count: 3, completed_at: 8000 }, }); -dispatch(SESSION_COMPLETION_UNREAD_CLEARED_KEY); +dispatch(rClearKey); const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); console.log(JSON.stringify({ handlers: _storageHandlers.length, From 221a241e61c3525d53a1208bdf80606f70551118 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 18:25:39 +0300 Subject: [PATCH 06/16] test: expose same-tick unread ordering races --- tests/test_unread_cross_client_sync.py | 44 ++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 45077dd3..9b702fa6 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -432,6 +432,50 @@ console.log(JSON.stringify({ ) +def test_later_clear_wins_after_same_millisecond_re_mark(): + """A clear must advance from the marker's logical order, not reuse the wall + clock tick and lose to the marker's synthetic ``clearedAt + 1`` timestamp.""" + out = _run_node(_script(""" +listed('Y'); +const A = makeClient(); +_now = 1000; +A.clearUnread('Y'); +A.markUnread('Y', 3); +const marked = A.hasUnread('Y'); +// The user visits again before the millisecond clock advances. +A.clearUnread('Y'); +console.log(JSON.stringify({ + marked, + final: A.hasUnread('Y'), + disk: readDisk(SESSION_COMPLETION_UNREAD_KEY), +})); +""")) + assert out["marked"] is True, "precondition: the same-ms re-mark must be visible" + assert out["final"] is False + assert out["disk"] == {}, "the later clear must remove the same-ms marker from disk" + + +def test_same_millisecond_clear_beats_marker_prepared_before_it(): + """A marker whose intent was prepared before a concurrent clear must lose, + even when both operations observe the same wall-clock millisecond.""" + out = _run_node(_script(""" +listed('Y'); +const A = makeClient(); +const B = makeClient(); +A.hasUnread('Y'); +B.hasUnread('Y'); +_now = 1000; +_onRead = () => { A.clearUnread('Y'); }; +B.markUnread('Y', 3); +console.log(JSON.stringify({ + marker: readDisk(SESSION_COMPLETION_UNREAD_KEY).Y || null, + visible: B.hasUnread('Y'), +})); +""")) + assert out["marker"] is None + assert out["visible"] is False + + def test_repair_removes_a_marker_that_lost_to_a_clear(): """A stale client's write can land after the clear. The repair must drop the losing marker from the store, or the dot returns on every client.""" From d32e558a8b5d8170bec3076bec9c976043574baa Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 18:28:38 +0300 Subject: [PATCH 07/16] fix: order unread transitions logically within a clock tick --- static/sessions.js | 51 +++++++++++++++++++------- tests/_unread_store_helpers.py | 2 + tests/test_unread_cross_client_sync.py | 20 ++++++---- 3 files changed, 52 insertions(+), 21 deletions(-) diff --git a/static/sessions.js b/static/sessions.js index ef5f1492..53cfcc91 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -611,7 +611,11 @@ function _getSessionCompletionUnread() { function _markerLosesToUnreadClear(marker, clearedAt) { const cleared = Number(clearedAt) || 0; if (!cleared) return false; - return (Number(marker && marker.completed_at) || 0) <= cleared; + return _sessionCompletionUnreadOrder(marker) <= cleared; +} + +function _sessionCompletionUnreadOrder(marker) { + return Number(marker && (marker.unread_order || marker.completed_at)) || 0; } function _sessionCompletionUnreadClearedKey(sid, clearedAt = null) { @@ -635,10 +639,17 @@ function _parseSessionCompletionUnreadClearedKey(key) { // 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() { + // 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 + // observes that clear and advances beyond it. + let keys = []; + try { + keys = Array.from({ length: localStorage.length }, (_, i) => localStorage.key(i)); + } catch (_) {} const cleared = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); try { - for (let i = 0; i < localStorage.length; i += 1) { - const key = localStorage.key(i); + for (const key of keys) { const parsed = _parseSessionCompletionUnreadClearedKey(key); if (!parsed) continue; const value = Math.max(parsed.version, Number(localStorage.getItem(key)) || 0); @@ -650,6 +661,18 @@ function _readSessionCompletionUnreadCleared() { return cleared; } +function _nextSessionCompletionUnreadOrder(sid, cleared = null) { + const clears = cleared || _readSessionCompletionUnreadCleared(); + const cached = _getSessionCompletionUnread()[sid]; + const stored = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_KEY)[sid]; + return Math.max( + Date.now(), + Number(clears[sid]) || 0, + _sessionCompletionUnreadOrder(cached), + _sessionCompletionUnreadOrder(stored) + ) + 1; +} + // Merge the persisted marker map with our cache under the tombstones. Markers // the store holds that a clear already outranks are dropped from the result, so // the caller can write the repaired map back and no reader sees them again. @@ -703,15 +726,12 @@ function _markSessionCompletionUnread(sid, messageCount = 0, meta = null) { if (!sid) return; const unread = _getSessionCompletionUnread(); const count = Number.isFinite(messageCount) ? Number(messageCount) : 0; - // If a clear and a genuine later completion share one millisecond, preserve - // event order rather than letting the clear's <= comparison erase the later - // marker. - const intendedAt = Date.now(); - const clearedAt = Number(_readSessionCompletionUnreadCleared()[sid]) || 0; - const completedAt = clearedAt && clearedAt <= intendedAt - ? Math.max(intendedAt, clearedAt + 1) - : intendedAt; - const entry = {message_count: count, completed_at: completedAt}; + const clearsAtIntent = _readSessionCompletionUnreadCleared(); + const entry = { + message_count: count, + completed_at: Date.now(), + unread_order: _nextSessionCompletionUnreadOrder(sid, clearsAtIntent), + }; // Cron markers carry source+profile so profile switches can clear only that // cross-profile leak without wiping ordinary chat completion unread (#5960). if (meta && typeof meta === 'object' && !Array.isArray(meta)) { @@ -811,10 +831,11 @@ function _clearSessionCompletionUnread(sid, recordIfMissing = true) { _readStoredJsonMap(SESSION_COMPLETION_UNREAD_KEY), sid); if (!stored && !recordIfMissing) return; } + const order = _nextSessionCompletionUnreadOrder(sid); delete unread[sid]; // Write the ordering fact before the marker map. This is required even when // the marker has been prepared by another client but has not reached storage. - _writeSessionCompletionUnreadCleared(sid, Date.now()); + _writeSessionCompletionUnreadCleared(sid, order); _saveSessionCompletionUnread(); } @@ -944,7 +965,9 @@ function _clearCronSessionCompletionUnreadForInactiveProfiles(activeProfile) { // Each cleared marker needs its durable ordering fact, or the merge in // _saveSessionCompletionUnread() (and a stale client's later save) would // restore it from the store. - _writeSessionCompletionUnreadCleared(clearedSids, Date.now()); + for (const sid of clearedSids) { + _writeSessionCompletionUnreadCleared(sid, _nextSessionCompletionUnreadOrder(sid)); + } _saveSessionCompletionUnread(); if (typeof renderSessionListFromCache === 'function') renderSessionListFromCache(); return true; diff --git a/tests/_unread_store_helpers.py b/tests/_unread_store_helpers.py index 6837f69e..c59788c1 100644 --- a/tests/_unread_store_helpers.py +++ b/tests/_unread_store_helpers.py @@ -21,6 +21,8 @@ _HELPER_NAMES = ( "_sessionListLoaded", "_mergeSessionViewedCounts", "_markerLosesToUnreadClear", + "_sessionCompletionUnreadOrder", + "_nextSessionCompletionUnreadOrder", "_sessionCompletionUnreadClearedKey", "_parseSessionCompletionUnreadClearedKey", "_readSessionCompletionUnreadCleared", diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 9b702fa6..03f2c399 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -113,6 +113,8 @@ _HELPER_FNS = [ "_sessionListLoaded", "_mergeSessionViewedCounts", "_markerLosesToUnreadClear", + "_sessionCompletionUnreadOrder", + "_nextSessionCompletionUnreadOrder", "_sessionCompletionUnreadClearedKey", "_parseSessionCompletionUnreadClearedKey", "_readSessionCompletionUnreadCleared", @@ -424,7 +426,11 @@ console.log(JSON.stringify({ bView: B.hasUnread('Y'), })); """)) - assert out["marker"] == {"message_count": 9, "completed_at": 3000}, ( + assert out["marker"] == { + "message_count": 9, + "completed_at": 3000, + "unread_order": 3001, + }, ( "a completion that postdates the clear must be kept" ) assert out["aView"] is True and out["bView"] is True, ( @@ -531,7 +537,7 @@ A.clearUnread('X'); const tombs = readAllClears(A); console.log(JSON.stringify({ tombs })); """)) - assert out["tombs"] == {"X": 2000, "Y": 2000}, ( + assert out["tombs"] == {"X": 2001, "Y": 2001}, ( "concurrent clears for different sessions must retain both tombstones" ) @@ -592,7 +598,7 @@ const marker = readDisk(SESSION_COMPLETION_UNREAD_KEY).Y || null; const tombstone = readAllClears(A).Y || null; console.log(JSON.stringify({ marker, tombstone })); """)) - assert out["tombstone"] == 2000, ( + assert out["tombstone"] == 2001, ( "a clear must persist its ordering fact even when the marker write is delayed" ) assert out["marker"] is None, "the completion that predates the visit must remain cleared" @@ -613,7 +619,7 @@ A.markUnread('Y', 1); A.clearUnread('Y'); console.log(JSON.stringify({ tombs: readAllClears(A) })); """)) - assert out["tombs"] == {"GONE": 1000, "Y": 2000}, ( + assert out["tombs"] == {"GONE": 1002, "Y": 2002}, ( "the versioned tombstone may be pruned, while its legacy compatibility " "fact remains available to already-open clients" ) @@ -641,12 +647,12 @@ console.log(JSON.stringify({ young: young, })); """)) - assert out["keptYoung"] == {"Y": 1000, "Z": out["young"]}, ( + assert out["keptYoung"] == {"Y": 1002, "Z": out["young"] + 2}, ( "a tombstone younger than the cap must survive a newer clear" ) assert out["afterAge"] == { - "Y": out["young"] + 8 * 24 * 60 * 60 * 1000, - "Z": out["young"], + "Y": out["young"] + 8 * 24 * 60 * 60 * 1000 + 2, + "Z": out["young"] + 2, }, ( "the old versioned record is dropped, while the rollout compatibility " "map remains available to older clients" From 1fd8d6fe2f402376c02b866d0d320806fce75eb0 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 18:29:19 +0300 Subject: [PATCH 08/16] test: require bounded one-way unread clear migration --- tests/test_unread_cross_client_sync.py | 37 +++++++++++++------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 03f2c399..722fdebe 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -542,28 +542,34 @@ console.log(JSON.stringify({ tombs })); ) -def test_new_clear_is_also_published_for_rolling_clients(): - """An already-open client on the previous PR head must see a clear created - by the new implementation in the legacy whole-map key.""" +def test_new_clear_does_not_dual_write_the_legacy_whole_map(): + """The independently addressable representation is authoritative. Keeping + dual-write compatibility would retain the old lost-update race forever.""" out = _run_node(_script(""" const A = makeClient(); A.writeClear('new', 2000); -console.log(JSON.stringify({ legacy: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), all: readAllClears(A) })); +console.log(JSON.stringify({ + legacyPresent: Object.prototype.hasOwnProperty.call(_store, SESSION_COMPLETION_UNREAD_CLEARED_KEY), + all: readAllClears(A), +})); """)) - assert out["legacy"] == {"new": 2000} + assert out["legacyPresent"] is False assert out["all"] == {"new": 2000} -def test_legacy_tombstone_map_is_kept_for_rolling_clients(): - """An already-open client on the previous PR head only understands the - whole-map key, so migration must not delete it during a rolling session.""" +def test_legacy_tombstone_map_is_migrated_then_removed(): + """A reloaded client imports existing clear facts once, then removes the + unbounded whole map instead of pretending concurrent old/new writes are safe.""" out = _run_node(_script(""" setDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY, { legacy: 1000 }); const A = makeClient(); A.writeClear('new', 2000); -console.log(JSON.stringify({ legacy: readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), all: readAllClears(A) })); +console.log(JSON.stringify({ + legacyPresent: Object.prototype.hasOwnProperty.call(_store, SESSION_COMPLETION_UNREAD_CLEARED_KEY), + all: readAllClears(A), +})); """)) - assert out["legacy"] == {"legacy": 1000, "new": 2000} + assert out["legacyPresent"] is False assert out["all"] == {"legacy": 1000, "new": 2000} @@ -619,10 +625,7 @@ A.markUnread('Y', 1); A.clearUnread('Y'); console.log(JSON.stringify({ tombs: readAllClears(A) })); """)) - assert out["tombs"] == {"GONE": 1002, "Y": 2002}, ( - "the versioned tombstone may be pruned, while its legacy compatibility " - "fact remains available to already-open clients" - ) + assert out["tombs"] == {"Y": 2002} def test_tombstones_are_capped_by_age(): @@ -652,11 +655,7 @@ console.log(JSON.stringify({ ) assert out["afterAge"] == { "Y": out["young"] + 8 * 24 * 60 * 60 * 1000 + 2, - "Z": out["young"] + 2, - }, ( - "the old versioned record is dropped, while the rollout compatibility " - "map remains available to older clients" - ) + }, "a tombstone older than the cap must be removed from storage" # ── The registration itself ────────────────────────────────────────────────── From 6d0a59da1012e18cbb0f192e8b847a4b4822bc41 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 18:36:37 +0300 Subject: [PATCH 09/16] fix(sessions): bound unread clear storage and stop list-based pruning Two defects share one cause: a fact was being discarded because a session was missing from the visible list. The sidebar filters by profile, project, and source, so an absent row may simply be hidden. Clear records were pruned on session existence, which deleted the ordering fact for any session outside the current filter and re-lit a dot the user had cleared. They are now pruned by age and supersession only. The same reasoning rules out pruning stored viewed counts by list membership, so a deletion records its own key instead. A client that still caches the acknowledgement now prunes it rather than writing it back. The previous clear representation is imported once and then removed. Keeping it would preserve both of its defects: concurrent clears could replace one another in the shared blob, and it grew with every session ever cleared. --- .../webui-run-state-consistency-contract.md | 49 ++++--- static/sessions.js | 124 ++++++++++++++---- tests/_unread_store_helpers.py | 5 + tests/test_unread_cross_client_sync.py | 66 +++++++++- 4 files changed, 199 insertions(+), 45 deletions(-) diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index d15a4a20..a822d3b2 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -170,22 +170,39 @@ writes. - **Viewed counts merge by maximum.** The fact is monotonic per session and additive across clients, so a save reads the store, merges its cache in with a - max, and writes only when the store does not already hold the merged result. A - count held only in a client's cache is written back only for a session the - sidebar list still shows, so a session deleted in another client is not - resurrected. -- **Completion markers are ordered by per-session tombstones.** Markers are - add/remove and cannot be max-ordered, so each clear records `clearedAt` under - `hermes-session-completion-unread-cleared:v1::`. Independent - immutable version keys mean clients clearing different sessions—or clearing the - same session in an interleaved operation—cannot replace newer ordering facts. A - marker that does not postdate its clear loses; a genuine later - completion still wins. During rolling upgrades, the previous unsuffixed - `hermes-session-completion-unread-cleared` map is both read and dual-written so - already-open clients on the prior revision observe new clears; it remains a - temporary compatibility store and is not pruned in this revision. Versioned - tombstones are bounded by session existence and a 7-day retention cap, and are - never part of the marker map consumers read. + max, and writes only when the store does not already hold the merged result. + A deletion records its own key under + `hermes-session-viewed-counts:deleted:v1:`; merges drop any count + whose session has a live deletion record, so a client that still caches the + acknowledgement prunes it instead of writing it back. List membership cannot + decide this, because the sidebar filters by profile, project, and source, so an + absent row is not evidence of deletion. Deletion records expire on the same + 7-day policy as clear records. +- **Completion markers are ordered by logical stamps, not wall clock.** + Markers are add/remove and cannot be max-ordered, so each clear records a stamp + under `hermes-session-completion-unread-cleared:v1::`. + Independent immutable keys mean clients clearing different sessions—or clearing + the same session in an interleaved operation—cannot replace newer ordering + facts, and a reader folds the maximum per session. Markers carry + `unread_order`: the greatest stamp the marker's creator had observed (its own + clear state, its cached and stored markers) plus one. A marker whose stamp does + not exceed its session's clear stamp loses, so a clear wins the tie when a + marker was prepared before it but written after; a completion that happens after + the clear observes it and stamps higher, so it still wins. Milliseconds are not + used for ordering: a clear and a genuine later completion can share one tick, and + a single observed clock cannot order them. +- **The previous clear representation is migrated once, then dropped.** The + unsuffixed `hermes-session-completion-unread-cleared` whole map is read, its + facts are imported as independent records, and the key is removed. Keeping it + would retain both of the defects it caused: concurrent clears could replace one + another in the shared blob, and the blob grew with every session ever cleared. + 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. - **Cross-client repair, not cache invalidation.** The `storage` listener routes a changed unread key (including any per-session clear key) back through the same merge instead of only dropping the local cache. A client that still holds an diff --git a/static/sessions.js b/static/sessions.js index 53cfcc91..f3bea7d6 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -346,6 +346,13 @@ function _clearComposerDraft(sid, text, files) { } const SESSION_VIEWED_COUNTS_KEY = 'hermes-session-viewed-counts'; +// A deleted session's acknowledgement must not return from a stale client's +// cache. Membership in the rendered list cannot decide this: the sidebar filters +// by profile, project, and source, so an absent row is not evidence of deletion. +// Each deletion therefore records its own key, and merges drop any entry whose +// session has one. Records are pruned by age so the store stays bounded. +const SESSION_VIEWED_COUNTS_DELETED_PREFIX = 'hermes-session-viewed-counts:deleted:v1:'; +const SESSION_VIEWED_COUNTS_DELETED_TTL_MS = 7 * 24 * 60 * 60 * 1000; const SESSION_COMPLETION_UNREAD_KEY = 'hermes-session-completion-unread'; // Clear ordering is stored per session so two clients clearing different // sessions never replace one another's tombstones with whole-map writes. The @@ -530,10 +537,19 @@ function _sessionListLoaded() { // is always kept in the live view regardless: dropping it here would lose the // acknowledgement for a session that still exists. function _mergeSessionViewedCounts(disk, cache) { + const deleted = _readSessionViewedCountDeletions(); + const gone = (sid) => Object.prototype.hasOwnProperty.call(deleted, sid); const counts = {}; - for (const sid of Object.keys(disk || {})) counts[sid] = Number(disk[sid]) || 0; let changed = false; + for (const sid of Object.keys(disk || {})) { + if (gone(sid)) { + changed = true; + continue; + } + counts[sid] = Number(disk[sid]) || 0; + } for (const sid of Object.keys(cache || {})) { + if (gone(sid)) continue; const ours = Number(cache[sid]) || 0; if (Object.prototype.hasOwnProperty.call(counts, sid)) { if (ours > counts[sid]) { @@ -550,6 +566,7 @@ function _mergeSessionViewedCounts(disk, cache) { const live = {}; for (const sid of Object.keys(counts)) live[sid] = counts[sid]; for (const sid of Object.keys(cache || {})) { + if (gone(sid)) continue; const ours = Number(cache[sid]) || 0; if (ours > (Object.prototype.hasOwnProperty.call(live, sid) ? live[sid] : 0)) { live[sid] = ours; @@ -558,6 +575,53 @@ function _mergeSessionViewedCounts(disk, cache) { return {counts, live, changed}; } +function _sessionViewedCountDeletedKey(sid) { + return `${SESSION_VIEWED_COUNTS_DELETED_PREFIX}${encodeURIComponent(String(sid))}`; +} + +// Read the deletion records, dropping any that have aged out. +function _readSessionViewedCountDeletions() { + const deleted = {}; + let keys = []; + try { + keys = Array.from({ length: localStorage.length }, (_, i) => localStorage.key(i)); + } catch (_) { + return deleted; + } + const now = Date.now(); + for (const key of keys) { + if (typeof key !== 'string' || !key.startsWith(SESSION_VIEWED_COUNTS_DELETED_PREFIX)) continue; + const encoded = key.slice(SESSION_VIEWED_COUNTS_DELETED_PREFIX.length); + let sid = ''; + try { + sid = decodeURIComponent(encoded); + } catch (_) { + continue; + } + if (!sid) continue; + let stamp = 0; + try { + stamp = Number(localStorage.getItem(key)) || 0; + } catch (_) {} + if (stamp && now - stamp > SESSION_VIEWED_COUNTS_DELETED_TTL_MS) { + try { localStorage.removeItem(key); } catch (_) {} + continue; + } + deleted[sid] = stamp; + } + return deleted; +} + +function _recordSessionViewedCountDeleted(sid) { + if (!sid) return; + try { + localStorage.setItem(_sessionViewedCountDeletedKey(sid), String(Date.now())); + } catch (_) { + // Without the record the deletion still removed the entry; the next delete + // or merge attempt records it again. + } +} + function _saveSessionViewedCounts() { try { const merged = _mergeSessionViewedCounts( @@ -774,34 +838,35 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { (Array.isArray(sidOrSids) ? sidOrSids : [sidOrSids]).map((sid) => String(sid)) ); const at = Number(clearedAt) || 0; - const legacyClears = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); + + // One-way migration. The previous representation kept every clear fact in one + // shared blob, so concurrent clears could replace one another and the blob + // only ever grew. Import those facts as independently addressable records and + // drop the blob. A client still running the previous revision will not observe + // clears recorded after this point; that is a reload boundary, not something a + // dual write could have fixed, because the shared blob cannot be written + // concurrently without losing an update. + const legacy = _readStoredJsonMap(SESSION_COMPLETION_UNREAD_CLEARED_KEY); + let migrated = true; + for (const [sid, value] of Object.entries(legacy)) { + const stamp = Number(value) || 0; + if (!stamp) continue; + try { + localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, stamp), String(stamp)); + } catch (_) { + migrated = false; + } + } for (const sid of justCleared) { try { localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, at), String(at)); } catch (_) {} - // Rolling compatibility: clients already open on the previous revision only - // understand the whole-map key. Publish new clears there too, while the - // immutable key remains authoritative for clients on this revision. - legacyClears[sid] = Math.max(Number(legacyClears[sid]) || 0, at); } - try { localStorage.setItem(SESSION_COMPLETION_UNREAD_CLEARED_KEY, JSON.stringify(legacyClears)); } catch (_) {} - - // Copy ordering facts from the old whole-map representation, but keep the - // legacy map until this PR no longer needs rolling compatibility. An - // already-open older client only understands that key; deleting it would let - // that client reassert stale markers. Keeping it also avoids a racy - // read/copy/remove migration losing a clear written by an older client. - for (const [sid, value] of Object.entries(legacyClears)) { - const version = Number(value) || 0; - if (!version) continue; - try { - localStorage.setItem( - _sessionCompletionUnreadClearedKey(sid, version), String(version)); - } catch (_) {} + if (Object.keys(legacy).length && migrated) { + try { localStorage.removeItem(SESSION_COMPLETION_UNREAD_CLEARED_KEY); } catch (_) {} } const cleared = _readSessionCompletionUnreadCleared(); - const listLoaded = _sessionListLoaded(); let keys = []; try { keys = Array.from({ length: localStorage.length }, (_, i) => localStorage.key(i)); @@ -812,10 +877,12 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { try { const version = Math.max(parsed.version, Number(localStorage.getItem(key)) || 0); const expired = at - version > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS; - const deleted = listLoaded && !justCleared.has(parsed.sid) - && !_sessionExistsForUnreadState(parsed.sid); const superseded = version < (Number(cleared[parsed.sid]) || 0); - if (expired || deleted || superseded) localStorage.removeItem(key); + // 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. + if (expired || superseded) localStorage.removeItem(key); } catch (_) {} } } @@ -978,6 +1045,10 @@ function _clearSessionViewedCount(sid) { const counts = _getSessionViewedCounts(); const hadCachedEntry = Object.prototype.hasOwnProperty.call(counts, sid); delete counts[sid]; + // Record the deletion before any merge, so this client cannot re-adopt the + // entry it is deleting and another client holding the same acknowledgement + // prunes it instead of writing it back. + _recordSessionViewedCountDeleted(sid); // A removal must actually leave disk, unlike the additive max-merge used on // the set path (which never deletes). Delete just this key from the persisted // map so entries a concurrent client added are preserved. The store is @@ -9373,7 +9444,10 @@ function _handleUnreadStorageEvent(e){ // as the live cache, and write back only what the store is missing. So this // client re-asserts the acknowledgement it still holds instead of dropping it // and re-reading a value the other client's write already lost. - if(e.key === SESSION_VIEWED_COUNTS_KEY) _saveSessionViewedCounts(); + if(e.key === SESSION_VIEWED_COUNTS_KEY + || (typeof e.key === 'string' && e.key.startsWith(SESSION_VIEWED_COUNTS_DELETED_PREFIX))){ + _saveSessionViewedCounts(); + } else if(e.key === SESSION_COMPLETION_UNREAD_KEY || e.key === SESSION_COMPLETION_UNREAD_CLEARED_KEY || (typeof e.key === 'string' diff --git a/tests/_unread_store_helpers.py b/tests/_unread_store_helpers.py index c59788c1..93ba4ed6 100644 --- a/tests/_unread_store_helpers.py +++ b/tests/_unread_store_helpers.py @@ -20,6 +20,9 @@ _HELPER_NAMES = ( "_sessionExistsForUnreadState", "_sessionListLoaded", "_mergeSessionViewedCounts", + "_sessionViewedCountDeletedKey", + "_readSessionViewedCountDeletions", + "_recordSessionViewedCountDeleted", "_markerLosesToUnreadClear", "_sessionCompletionUnreadOrder", "_nextSessionCompletionUnreadOrder", @@ -33,6 +36,8 @@ _HELPER_NAMES = ( # Declared from the module source so a harness cannot drift from the values the # module uses. Harnesses keep their own stub keys for the two older stores. _CONST_NAMES = ( + "SESSION_VIEWED_COUNTS_DELETED_PREFIX", + "SESSION_VIEWED_COUNTS_DELETED_TTL_MS", "SESSION_COMPLETION_UNREAD_CLEARED_KEY", "SESSION_COMPLETION_UNREAD_CLEARED_PREFIX", "SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS", diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 722fdebe..743b7d26 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -112,6 +112,9 @@ _HELPER_FNS = [ "_sessionExistsForUnreadState", "_sessionListLoaded", "_mergeSessionViewedCounts", + "_sessionViewedCountDeletedKey", + "_readSessionViewedCountDeletions", + "_recordSessionViewedCountDeleted", "_markerLosesToUnreadClear", "_sessionCompletionUnreadOrder", "_nextSessionCompletionUnreadOrder", @@ -141,6 +144,10 @@ const localStorage = {{ get length() {{ return Object.keys(_store).length; }}, }}; const SESSION_VIEWED_COUNTS_KEY = {VIEWED_KEY!r}; +const SESSION_VIEWED_COUNTS_DELETED_PREFIX = `${{SESSION_VIEWED_COUNTS_KEY}}:deleted:v1:`; +const SESSION_VIEWED_COUNTS_DELETED_TTL_MS = 7 * 24 * 60 * 60 * 1000; +const readViewedDeletions = () => + Object.keys(_store).filter((k) => k.startsWith(SESSION_VIEWED_COUNTS_DELETED_PREFIX)); const SESSION_COMPLETION_UNREAD_KEY = {UNREAD_KEY!r}; const SESSION_COMPLETION_UNREAD_CLEARED_KEY = {CLEARED_KEY!r}; const SESSION_COMPLETION_UNREAD_CLEARED_PREFIX = `${{SESSION_COMPLETION_UNREAD_CLEARED_KEY}}:v1:`; @@ -203,6 +210,9 @@ function makeClient() {{ handleStorage: (typeof _handleUnreadStorageEvent === 'function') ? _handleUnreadStorageEvent : () => {{}}, + deletions: (typeof _readSessionViewedCountDeletions === 'function') + ? _readSessionViewedCountDeletions + : () => ({{}}), }}; }} """ @@ -375,6 +385,53 @@ console.log(JSON.stringify({ # ── Completion unread: the later event wins ────────────────────────────────── +def test_stale_client_cannot_resurrect_a_deleted_sessions_count(): + """Deleting a session records its own fact, so a client that still caches the + acknowledgement prunes it instead of writing it back — list membership cannot + decide this, because the sidebar filters by profile, project, and source.""" + out = _run_node(_script(""" +listed('S', 'T'); +const A = makeClient(); +const B = makeClient(); +A.setViewed('S', 5); +B.viewed('S'); +A.clearViewed('S'); +B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); +B.setViewed('T', 1); +console.log(JSON.stringify({ + disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + deletions: readViewedDeletions().length, + bView: B.viewed('S'), +})); +""")) + assert out["disk"] == {"T": 1}, ( + "a deleted session's count must not come back from a stale client" + ) + assert out["deletions"] == 1 + assert out["bView"] is None + + +def test_viewed_count_deletion_records_are_pruned_by_age(): + """The deletion records are bounded like the other stores: a later merge + drops one that has aged out.""" + out = _run_node(_script(""" +listed('S', 'X'); +const A = makeClient(); +const young = (() => { A.clearViewed('S'); return readViewedDeletions().length; })(); +const keptYoung = (() => { A.setViewed('X', 2); return readViewedDeletions().length; })(); +_now = 1000 + 8 * 24 * 60 * 60 * 1000; +A.setViewed('X', 3); +console.log(JSON.stringify({ + young: young, + keptYoung: keptYoung, + afterAge: readViewedDeletions().length, +})); +""")) + assert out["young"] == 1 + assert out["keptYoung"] == 1, "a fresh deletion record must survive an unrelated merge" + assert out["afterAge"] == 0, "an expired deletion record must be pruned" + + def test_stale_client_cannot_resurrect_a_cleared_completion_marker(): """The field symptom: a marker cleared by opening the chat must not come back when another client that still holds it saves later.""" @@ -610,9 +667,10 @@ console.log(JSON.stringify({ marker, tombstone })); assert out["marker"] is None, "the completion that predates the visit must remain cleared" -def test_tombstones_are_pruned_when_their_session_is_gone(): - """Tombstones are keyed by session id; one whose session no longer exists - must not accumulate.""" +def test_clear_records_survive_a_session_missing_from_the_list(): + """A session absent from the visible list is not proof of deletion: the list + is filtered by profile, project, and source. Dropping its clear record would + re-light a dot the user already cleared, so age is the only pruning signal.""" out = _run_node(_script(""" listed('Y', 'GONE'); const A = makeClient(); @@ -625,7 +683,7 @@ A.markUnread('Y', 1); A.clearUnread('Y'); console.log(JSON.stringify({ tombs: readAllClears(A) })); """)) - assert out["tombs"] == {"Y": 2002} + assert out["tombs"] == {"GONE": 1002, "Y": 2002} def test_tombstones_are_capped_by_age(): From ae0a798f132cb91e36d8a7c6c8a930125fd7345f Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 18:44:26 +0300 Subject: [PATCH 10/16] test(storage): widen the viewed-count clear slice to cover the function The assertion slices a fixed 1600 characters from the start of _clearSessionViewedCount, so growing the clear path pushes its last statement out of the window and fails an assertion about persistence that is still true. Bound the slice to the function instead of to an arbitrary width. --- tests/test_issue2389_storage_pressure.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_issue2389_storage_pressure.py b/tests/test_issue2389_storage_pressure.py index 5ab8baac..cd9a623e 100644 --- a/tests/test_issue2389_storage_pressure.py +++ b/tests/test_issue2389_storage_pressure.py @@ -48,7 +48,10 @@ def test_deleted_sessions_prune_all_session_tracking_maps(): def test_session_viewed_count_prune_is_best_effort_and_persists_when_changed(): - viewed_block = _function_block(SESSIONS_SRC, "_clearSessionViewedCount") + # The window is a source slice, not a semantic bound: keep it wide enough to + # cover the whole function so unrelated edits to the clear path do not fail + # this assertion by pushing its last statement out of range. + viewed_block = _function_block(SESSIONS_SRC, "_clearSessionViewedCount", window=2200) assert "Object.prototype.hasOwnProperty.call(counts, sid)" in viewed_block assert "delete counts[sid]" in viewed_block assert "_saveSessionViewedCounts()" in viewed_block From d61d7212fbfd310e8d274a47a55ffa2930dbffab Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 18:56:21 +0300 Subject: [PATCH 11/16] fix(sessions): merge completion markers by logical order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When both the store and the cache held a marker for one session, the merge chose between them by completed_at while the clear comparison used unread_order. Two completions in the same millisecond therefore kept the earlier marker and its message count and cron profile metadata. Compare unread_order on both sides, matching the ordering the rest of the unread state uses, and record in the RFC that a deletion record's expiry can let a very long-lived tab re-add a count for a session deleted more than seven days ago — bounded and invisible, since that session is no longer listed. --- .../webui-run-state-consistency-contract.md | 5 +++- static/sessions.js | 7 ++++-- tests/test_unread_cross_client_sync.py | 25 +++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index a822d3b2..d7da1ec3 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -177,7 +177,10 @@ writes. acknowledgement prunes it instead of writing it back. List membership cannot decide this, because the sidebar filters by profile, project, and source, so an absent row is not evidence of deletion. Deletion records expire on the same - 7-day policy as clear records. + 7-day policy as clear records, so a tab left open longer than that can re-add a + count for a session deleted more than seven days earlier. That consequence is + bounded and invisible: the session is no longer listed, so the retained entry + produces no indicator, and it is re-examined only on the next deletion or clear. - **Completion markers are ordered by logical stamps, not wall clock.** Markers are add/remove and cannot be max-ordered, so each clear records a stamp under `hermes-session-completion-unread-cleared:v1::`. diff --git a/static/sessions.js b/static/sessions.js index f3bea7d6..30005822 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -755,8 +755,11 @@ function _mergeSessionCompletionUnread(disk, cache, cleared) { const ours = cache[sid]; if (_markerLosesToUnreadClear(ours, (cleared || {})[sid])) continue; if (Object.prototype.hasOwnProperty.call(markers, sid)) { - const theirs = Number((markers[sid] || {}).completed_at) || 0; - if ((Number(ours && ours.completed_at) || 0) > theirs) { + // Both sides hold a marker: the later logical operation wins, which is the + // same order the clear comparison uses. Comparing completed_at here would + // keep the earlier of two markers written in one millisecond and leave that + // one's message count or cron profile metadata for later cleanup. + if (_sessionCompletionUnreadOrder(ours) > _sessionCompletionUnreadOrder(markers[sid])) { markers[sid] = ours; changed = true; } diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 743b7d26..1e40706d 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -432,6 +432,31 @@ console.log(JSON.stringify({ assert out["afterAge"] == 0, "an expired deletion record must be pruned" +def test_marker_merge_keeps_the_later_operation_within_one_tick(): + """Two clients can complete a session in the same millisecond. The merge must + pick the later logical operation, not compare wall-clock completion times, + or the retained marker keeps the older message count and metadata.""" + out = _run_node(_script(""" +listed('Y'); +const A = makeClient(); +const B = makeClient(); +_now = 1000; +A.markUnread('Y', 3); +B.markUnread('Y', 7); +const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); +console.log(JSON.stringify({ + count: (disk.Y || {}).message_count || null, + completedAt: (disk.Y || {}).completed_at || null, +})); +""")) + assert out["count"] == 7, ( + "the later completion must win even though both markers share a millisecond" + ) + assert out["completedAt"] == 1000, ( + "the wall-clock completion time is still recorded for display and pruning" + ) + + def test_stale_client_cannot_resurrect_a_cleared_completion_marker(): """The field symptom: a marker cleared by opening the chat must not come back when another client that still holds it saves later.""" From fc6efc008afe4b484265721cca16e61e18699fec Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Wed, 16 Sep 2026 05:52:02 +0300 Subject: [PATCH 12/16] fix: preserve unread state across transcript resets --- api/models.py | 10 ++ api/session_ops.py | 2 + api/webui_session_db.py | 2 + .../webui-run-state-consistency-contract.md | 28 +++- static/sessions.js | 137 ++++++++++++++---- tests/_unread_store_helpers.py | 7 +- tests/test_465_session_branching.py | 2 +- .../test_issue5273_active_done_viewed_sync.py | 4 +- .../test_issue6611_regeneration_settlement.py | 10 ++ ...est_issue6911_intentional_shrink_backup.py | 30 ++++ ...t_issue856_background_completion_unread.py | 4 +- tests/test_session_unread_dot_on_visit.py | 7 +- tests/test_unread_cross_client_sync.py | 125 ++++++++++++++-- ...ebui_run_state_consistency_contract_rfc.py | 9 ++ 14 files changed, 319 insertions(+), 58 deletions(-) diff --git a/api/models.py b/api/models.py index 775124dc..3de149ba 100644 --- a/api/models.py +++ b/api/models.py @@ -1247,6 +1247,8 @@ class Session: truncation_boundary=None, clear_generation=None, intentional_shrink_generation=None, + transcript_generation: int=0, + transcript_generation_baseline: int=0, gateway_routing=None, gateway_routing_history=None, llm_title_generated: bool=False, manual_title: bool=False, @@ -1344,6 +1346,10 @@ class Session: self.truncation_boundary = truncation_boundary self.clear_generation = clear_generation self.intentional_shrink_generation = intentional_shrink_generation + self.transcript_generation = max(0, _parse_nonnegative_int(transcript_generation) or 0) + self.transcript_generation_baseline = max( + 0, _parse_nonnegative_int(transcript_generation_baseline) or 0 + ) self.gateway_routing = gateway_routing if isinstance(gateway_routing, dict) else None self.gateway_routing_history = gateway_routing_history if isinstance(gateway_routing_history, list) else [] self.llm_title_generated = bool(llm_title_generated) @@ -1430,6 +1436,8 @@ class Session: 'truncation_boundary', 'clear_generation', 'intentional_shrink_generation', + 'transcript_generation', + 'transcript_generation_baseline', 'gateway_routing', 'gateway_routing_history', 'llm_title_generated', 'manual_title', 'parent_session_id', 'worktree_path', 'worktree_branch', 'worktree_repo_root', 'worktree_created_at', @@ -1768,6 +1776,8 @@ class Session: 'created_at': self.created_at, 'updated_at': self.updated_at, 'last_message_at': last_message_at, + 'transcript_generation': self.transcript_generation, + 'transcript_generation_baseline': self.transcript_generation_baseline, 'pinned': self.pinned, 'archived': self.archived, 'project_id': self.project_id, diff --git a/api/session_ops.py b/api/session_ops.py index c03fc09f..846075c6 100644 --- a/api/session_ops.py +++ b/api/session_ops.py @@ -608,6 +608,8 @@ def _stamp_intentional_shrink_generation(session, old_message_count: int, new_me if new_message_count >= old_message_count: return False session.intentional_shrink_generation = uuid.uuid4().hex + session.transcript_generation = max(0, int(getattr(session, 'transcript_generation', 0) or 0)) + 1 + session.transcript_generation_baseline = max(0, int(new_message_count)) return True diff --git a/api/webui_session_db.py b/api/webui_session_db.py index fa699a06..68ce2c1f 100644 --- a/api/webui_session_db.py +++ b/api/webui_session_db.py @@ -57,6 +57,8 @@ _METADATA_FIELDS = frozenset( "compression_recovery_action", "truncation_watermark", "truncation_boundary", + "transcript_generation", + "transcript_generation_baseline", "gateway_routing", "gateway_routing_history", "llm_title_generated", diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index d7da1ec3..aae3d695 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -165,13 +165,21 @@ writes. | Store | Key | Semantics | |---|---|---| -| Viewed counts | `hermes-session-viewed-counts` | `sid -> N`, meaning "seen at least up to N messages" | +| Viewed counts | `hermes-session-viewed-counts` | `sid -> {message_count, transcript_generation}`, meaning "seen up to N messages in this transcript generation" | | Completion markers | `hermes-session-completion-unread` | `sid -> {message_count, completed_at, ...}` behind the visible dot | -- **Viewed counts merge by maximum.** The fact is monotonic per session and - additive across clients, so a save reads the store, merges its cache in with a - max, and writes only when the store does not already hold the merged result. - A deletion records its own key under +- **Viewed counts are generation-scoped.** Session mutation routes increment the + persisted `transcript_generation` whenever edit, regenerate, retry, undo, clear, + or truncate reduces the visible transcript, and record the retained count as + `transcript_generation_baseline`. A newer generation replaces an older one even + when its count is lower; counts are monotonic only within one generation and + merge by maximum there. A client first observing a newer generation acknowledges + only that retained baseline, so messages added after the shrink remain unread + even when the shrink and later growth arrive in one coalesced sidebar refresh. + Legacy numeric records are generation zero and migrate to the structured + representation on their next save. This prevents an old pre-truncate high-water + mark from masking messages added after a transcript reset. A deletion records + its own key under `hermes-session-viewed-counts:deleted:v1:`; merges drop any count whose session has a live deletion record, so a client that still caches the acknowledgement prunes it instead of writing it back. List membership cannot @@ -205,7 +213,15 @@ writes. 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. + 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, + 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 + same 7-day policy. - **Cross-client repair, not cache invalidation.** The `storage` listener routes a changed unread key (including any per-session clear key) back through the same merge instead of only dropping the local cache. A client that still holds an diff --git a/static/sessions.js b/static/sessions.js index 30005822..8efd006d 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -372,6 +372,7 @@ const SESSION_PROFILE_COUNTS_KEY = 'hermes-session-profile-counts'; let _sessionProfileCounts = null; let _sessionViewedCounts = null; let _sessionCompletionUnread = null; +let _sessionCompletionUnreadClearedMemory = {}; let _sessionObservedStreaming = null; const _sessionStreamingById = new Map(); const _sessionListSnapshotById = new Map(); @@ -528,14 +529,52 @@ function _sessionListLoaded() { && _sessionListSnapshotById.size > 0; } -// A viewed count is "seen at least up to N messages": monotonic per session and -// additive across clients, so merging is a max that never rolls a count back and -// never drops another client's entry. A value that exists only in our cache is -// written back only for a session the list still shows (see -// _sessionExistsForUnreadState) so a session deleted in another client, and -// already pruned from disk by _clearSessionViewedCount, is not resurrected. It -// is always kept in the live view regardless: dropping it here would lose the -// acknowledgement for a session that still exists. +// Viewed acknowledgements are generation-scoped. Transcript mutation routes +// increment `transcript_generation` whenever the visible message list shrinks; +// counts are monotonic only within one generation. Legacy numeric entries belong +// to generation zero and are migrated on their next save. +function _sessionViewedCountRecord(value, transcriptGeneration = 0) { + if (value && typeof value === 'object' && !Array.isArray(value)) { + return { + message_count: Number(value.message_count) || 0, + transcript_generation: Math.max(0, Number(value.transcript_generation) || 0), + }; + } + return { + message_count: Number(value) || 0, + transcript_generation: Math.max(0, Number(transcriptGeneration) || 0), + }; +} + +function _sessionViewedCountValue(value) { + return _sessionViewedCountRecord(value).message_count; +} + +function _sessionViewedRecordWins(candidate, current) { + const ours = _sessionViewedCountRecord(candidate); + const theirs = _sessionViewedCountRecord(current); + if (ours.transcript_generation !== theirs.transcript_generation) { + return ours.transcript_generation > theirs.transcript_generation; + } + return ours.message_count > theirs.message_count; +} + +function _sessionTranscriptGenerationForUnread(sid, explicit = null) { + if (explicit !== null && explicit !== undefined && Number.isFinite(Number(explicit))) { + return Math.max(0, Number(explicit) || 0); + } + let active = null; + try { + active = (S && S.session && S.session.session_id === sid) ? S.session : null; + } catch (_) {} + const snapshot = (typeof _sessionListSnapshotById !== 'undefined' && _sessionListSnapshotById) + ? _sessionListSnapshotById.get(sid) : null; + const listed = (typeof _allSessions !== 'undefined' && Array.isArray(_allSessions)) + ? _allSessions.find((s) => s && s.session_id === sid) : null; + const session = active || snapshot || listed; + return Math.max(0, Number(session && session.transcript_generation) || 0); +} + function _mergeSessionViewedCounts(disk, cache) { const deleted = _readSessionViewedCountDeletions(); const gone = (sid) => Object.prototype.hasOwnProperty.call(deleted, sid); @@ -546,19 +585,20 @@ function _mergeSessionViewedCounts(disk, cache) { changed = true; continue; } - counts[sid] = Number(disk[sid]) || 0; + const record = _sessionViewedCountRecord(disk[sid]); + counts[sid] = record; + if (!disk[sid] || typeof disk[sid] !== 'object' || Array.isArray(disk[sid])) changed = true; } for (const sid of Object.keys(cache || {})) { if (gone(sid)) continue; - const ours = Number(cache[sid]) || 0; + const ours = _sessionViewedCountRecord(cache[sid]); if (Object.prototype.hasOwnProperty.call(counts, sid)) { - if (ours > counts[sid]) { + if (_sessionViewedRecordWins(ours, counts[sid])) { counts[sid] = ours; changed = true; } continue; } - if (!ours) continue; if (_sessionListLoaded() && !_sessionExistsForUnreadState(sid)) continue; counts[sid] = ours; changed = true; @@ -567,8 +607,8 @@ function _mergeSessionViewedCounts(disk, cache) { for (const sid of Object.keys(counts)) live[sid] = counts[sid]; for (const sid of Object.keys(cache || {})) { if (gone(sid)) continue; - const ours = Number(cache[sid]) || 0; - if (ours > (Object.prototype.hasOwnProperty.call(live, sid) ? live[sid] : 0)) { + const ours = _sessionViewedCountRecord(cache[sid]); + if (!Object.prototype.hasOwnProperty.call(live, sid) || _sessionViewedRecordWins(ours, live[sid])) { live[sid] = ours; } } @@ -640,21 +680,26 @@ function _saveSessionViewedCounts() { } } -function _setSessionViewedCount(sid, messageCount = 0) { +function _setSessionViewedCount(sid, messageCount = 0, transcriptGeneration = null) { if (!sid) return; const counts = _getSessionViewedCounts(); const next = Number.isFinite(messageCount) ? Number(messageCount) : 0; + const generation = _sessionTranscriptGenerationForUnread(sid, transcriptGeneration); + const incoming = _sessionViewedCountRecord(next, generation); const hadCount = Object.prototype.hasOwnProperty.call(counts, sid); - const previous = Number(counts[sid]) || 0; - // A viewed count records "seen at least up to N messages" — keep it monotonic. - // A transient lower count (e.g. a list snapshot mid-compaction) must not - // resurrect a cleared unread dot before the save below. - counts[sid] = Math.max(previous, next); + const previous = hadCount ? _sessionViewedCountRecord(counts[sid]) : null; + if (!previous || _sessionViewedRecordWins(incoming, previous)) { + counts[sid] = incoming; + } else { + counts[sid] = previous; + } _saveSessionViewedCounts(); - // Advancing the acknowledgement records a clear even if another client has - // only prepared (not persisted) its older completion marker. Repeated polling - // at the same count does not refresh the tombstone or emit storage events. - _clearSessionCompletionUnread(sid, !hadCount || next > previous); + // A generation change or count advance acknowledges completion state even if + // another client prepared an older marker before this save. + const advanced = !previous + || generation > previous.transcript_generation + || (generation === previous.transcript_generation && next > previous.message_count); + _clearSessionCompletionUnread(sid, advanced); } function _getSessionCompletionUnread() { @@ -712,11 +757,22 @@ 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); if (!parsed) continue; - const value = Math.max(parsed.version, Number(localStorage.getItem(key)) || 0); + const storedValue = Number(localStorage.getItem(key)) || 0; + const value = parsed.version || storedValue; if (value > (Number(cleared[parsed.sid]) || 0)) cleared[parsed.sid] = value; } } catch (_) { @@ -841,6 +897,12 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { (Array.isArray(sidOrSids) ? sidOrSids : [sidOrSids]).map((sid) => String(sid)) ); const at = Number(clearedAt) || 0; + for (const sid of justCleared) { + const previous = _sessionCompletionUnreadClearedMemory[sid]; + if (at > (Number(previous && previous.order) || 0)) { + _sessionCompletionUnreadClearedMemory[sid] = {order: at, recorded_at: Date.now()}; + } + } // One-way migration. The previous representation kept every clear fact in one // shared blob, so concurrent clears could replace one another and the blob @@ -855,14 +917,14 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { const stamp = Number(value) || 0; if (!stamp) continue; try { - localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, stamp), String(stamp)); + localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, stamp), String(Date.now())); } catch (_) { migrated = false; } } for (const sid of justCleared) { try { - localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, at), String(at)); + localStorage.setItem(_sessionCompletionUnreadClearedKey(sid, at), String(Date.now())); } catch (_) {} } if (Object.keys(legacy).length && migrated) { @@ -878,8 +940,9 @@ function _writeSessionCompletionUnreadCleared(sidOrSids, clearedAt) { const parsed = _parseSessionCompletionUnreadClearedKey(key); if (!parsed) continue; try { - const version = Math.max(parsed.version, Number(localStorage.getItem(key)) || 0); - const expired = at - version > SESSION_COMPLETION_UNREAD_CLEARED_TTL_MS; + 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, @@ -1123,12 +1186,22 @@ function _hasUnreadForSession(s) { if (!s || !s.session_id) return false; if (_hasSessionCompletionUnread(s.session_id)) return true; const counts = _getSessionViewedCounts(); + const generation = _sessionTranscriptGenerationForUnread(s.session_id, s.transcript_generation); if (!Object.prototype.hasOwnProperty.call(counts, s.session_id)) { - _setSessionViewedCount(s.session_id, Number(s.message_count || 0)); + _setSessionViewedCount(s.session_id, Number(s.message_count || 0), generation); return false; } - if (!Number.isFinite(s.message_count)) return false; - return s.message_count > Number(counts[s.session_id] || 0); + const viewed = _sessionViewedCountRecord(counts[s.session_id]); + if (generation > viewed.transcript_generation) { + const baseline = Math.max(0, Math.min( + Number(s.message_count || 0), + Number(s.transcript_generation_baseline) || 0 + )); + _setSessionViewedCount(s.session_id, baseline, generation); + return Number.isFinite(s.message_count) && s.message_count > baseline; + } + if (generation < viewed.transcript_generation || !Number.isFinite(s.message_count)) return false; + return s.message_count > viewed.message_count; } // Keep the sidebar polling snapshot current for a just-visited session so a diff --git a/tests/_unread_store_helpers.py b/tests/_unread_store_helpers.py index 93ba4ed6..0d187e64 100644 --- a/tests/_unread_store_helpers.py +++ b/tests/_unread_store_helpers.py @@ -19,6 +19,10 @@ _HELPER_NAMES = ( "_readStoredJsonMap", "_sessionExistsForUnreadState", "_sessionListLoaded", + "_sessionViewedCountRecord", + "_sessionViewedCountValue", + "_sessionViewedRecordWins", + "_sessionTranscriptGenerationForUnread", "_mergeSessionViewedCounts", "_sessionViewedCountDeletedKey", "_readSessionViewedCountDeletions", @@ -83,6 +87,7 @@ def _const_source(name: str) -> str: # Ready to interpolate into a node harness. BLOCK = "\n".join( - [_const_source(name) for name in _CONST_NAMES] + ["var _sessionCompletionUnreadClearedMemory = {};\n"] + + [_const_source(name) for name in _CONST_NAMES] + [_function_source(name) for name in _HELPER_NAMES if f"function {name}(" in SESSIONS_JS] ) diff --git a/tests/test_465_session_branching.py b/tests/test_465_session_branching.py index e792c0c4..2d8f1e8a 100644 --- a/tests/test_465_session_branching.py +++ b/tests/test_465_session_branching.py @@ -610,7 +610,7 @@ def test_session_compact_includes_parent(): # method name and first parameter without pinning formatting. compact_def_match = re.search(r"def compact\(\s*self", src) assert compact_def_match, "Could not find compact() method" - snippet = src[compact_def_match.start():compact_def_match.start() + 3000] + snippet = src[compact_def_match.start():compact_def_match.start() + 3500] assert "'parent_session_id'" in snippet, \ "compact() should include parent_session_id" diff --git a/tests/test_issue5273_active_done_viewed_sync.py b/tests/test_issue5273_active_done_viewed_sync.py index da5d8594..26f52552 100644 --- a/tests/test_issue5273_active_done_viewed_sync.py +++ b/tests/test_issue5273_active_done_viewed_sync.py @@ -161,7 +161,7 @@ def _run_done_compaction_harness( _markSessionCompletedInList(completedSession, activeSid); const cacheRow=_allSessions.find(s=>s&&s.session_id===completedSid); const unreadAfterCacheUpdate=_hasUnreadForSession(cacheRow); - const viewedCountAfterCacheUpdate=_getSessionViewedCounts()[completedSid] ?? null; + const viewedCountAfterCacheUpdate=_sessionViewedCountValue(_getSessionViewedCounts()[completedSid]); if ({str(is_session_viewed).lower()}) {{ _markSessionViewed(completedSid, completedMessageCount); }} @@ -172,7 +172,7 @@ def _run_done_compaction_harness( unreadAfterActiveBranchSync:_hasUnreadForSession(cacheRow), hasCompletionUnread:_hasSessionCompletionUnread(completedSid), completionUnreadMessageCount:_getSessionCompletionUnread()[completedSid]?.message_count ?? null, - viewedCount:_getSessionViewedCounts()[completedSid] ?? null, + viewedCount:_sessionViewedCountValue(_getSessionViewedCounts()[completedSid]), cacheRow, }})); """ diff --git a/tests/test_issue6611_regeneration_settlement.py b/tests/test_issue6611_regeneration_settlement.py index 77bb76fd..7ba52ec0 100644 --- a/tests/test_issue6611_regeneration_settlement.py +++ b/tests/test_issue6611_regeneration_settlement.py @@ -30,6 +30,16 @@ def test_recovered_prefix_survives_regeneration_truncation(): assert [row["content"] for row in session.messages] == ["old", "old answer", "latest"] +def test_regeneration_advances_transcript_generation_when_it_shrinks(): + session = _session() + session.transcript_generation = 7 + plan = plan_regeneration(session) + + assert apply_regeneration_plan(session, plan) + + assert session.transcript_generation == 8 + + def test_stale_revision_rejects_without_mutation(): session = _session() before = copy.deepcopy(session.__dict__) diff --git a/tests/test_issue6911_intentional_shrink_backup.py b/tests/test_issue6911_intentional_shrink_backup.py index 1cd8d1db..fc437a99 100644 --- a/tests/test_issue6911_intentional_shrink_backup.py +++ b/tests/test_issue6911_intentional_shrink_backup.py @@ -162,6 +162,36 @@ def test_generation_save_load_round_trip_is_in_metadata_prefix(monkeypatch, tmp_ assert metadata.intentional_shrink_generation == generation +def test_transcript_generation_round_trips_and_advances_only_on_shrink(monkeypatch, tmp_path): + models, _session_dir = _seed_session_dir(monkeypatch, tmp_path) + from api.session_ops import truncate_session_at_keep + from api.webui_session_db import WebUIJsonSessionDB + + session = models.Session( + session_id="issue6911_transcript_generation", + messages=_history(), + transcript_generation=4, + ) + session.save() + + truncate_session_at_keep(session, len(session.messages)) + assert session.transcript_generation == 4 + truncate_session_at_keep(session, 2) + assert session.transcript_generation == 5 + assert session.transcript_generation_baseline == 2 + session.save() + + loaded = models.Session.load(session.session_id) + metadata = models.Session.load_metadata_only(session.session_id) + listed = WebUIJsonSessionDB().list_sessions() + assert loaded.transcript_generation == 5 + assert loaded.transcript_generation_baseline == 2 + assert metadata.transcript_generation == 5 + assert metadata.transcript_generation_baseline == 2 + assert listed[0]["transcript_generation"] == 5 + assert listed[0]["transcript_generation_baseline"] == 2 + + def test_no_actual_message_shrink_keeps_generation(monkeypatch, tmp_path): models, _session_dir = _seed_session_dir(monkeypatch, tmp_path) from api.session_ops import truncate_session_at_keep diff --git a/tests/test_issue856_background_completion_unread.py b/tests/test_issue856_background_completion_unread.py index 0368a108..f11349ba 100644 --- a/tests/test_issue856_background_completion_unread.py +++ b/tests/test_issue856_background_completion_unread.py @@ -52,7 +52,7 @@ def test_background_completion_unread_uses_explicit_marker_not_message_delta(): 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 > Number") + 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, ( @@ -561,6 +561,6 @@ def test_historical_sessions_are_not_marked_unread_on_list_render(): 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));" in has_unread_block, ( + 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" ) diff --git a/tests/test_session_unread_dot_on_visit.py b/tests/test_session_unread_dot_on_visit.py index 789062f9..b897f106 100644 --- a/tests/test_session_unread_dot_on_visit.py +++ b/tests/test_session_unread_dot_on_visit.py @@ -212,7 +212,7 @@ const before = Object.prototype.hasOwnProperty.call(_getSessionCompletionUnread( _acknowledgeSessionVisit('open', 5, 10); const after = Object.prototype.hasOwnProperty.call(_getSessionCompletionUnread(), 'open'); const snap = _sessionListSnapshotById.get('open'); -console.log(JSON.stringify({{before, after, repaints, viewed: _getSessionViewedCounts()['open'], snap}})); +console.log(JSON.stringify({{before, after, repaints, viewed: _sessionViewedCountValue(_getSessionViewedCounts()['open']), snap}})); """ out = _run_node(script) assert out["before"] is True, "precondition: marker seeded" @@ -419,12 +419,13 @@ function _hasMarker() {{ _releaseApi({{ session: {{ session_id: 'open', message_count: 6, messages: [{{ role: 'assistant', content: 'x' }}] }} }}); await p; const markerAfter = _hasMarker(); - const viewed = _getSessionViewedCounts()['open']; + const viewedRecord = _getSessionViewedCounts()['open']; + const viewed = viewedRecord === undefined ? null : _sessionViewedCountValue(viewedRecord); console.log(JSON.stringify({{ apiIssued, markerBefore, markerAfter, - viewed: viewed === undefined ? null : viewed, + viewed, }})); }})(); """ diff --git a/tests/test_unread_cross_client_sync.py b/tests/test_unread_cross_client_sync.py index 1e40706d..ab7b38c4 100644 --- a/tests/test_unread_cross_client_sync.py +++ b/tests/test_unread_cross_client_sync.py @@ -104,6 +104,7 @@ _CLIENT_FNS = [ "_markSessionCompletionUnread", "_clearSessionCompletionUnread", "_hasSessionCompletionUnread", + "_hasUnreadForSession", ] # Helpers that carry the merge/tombstone rules; injected when present. @@ -111,6 +112,10 @@ _HELPER_FNS = [ "_readStoredJsonMap", "_sessionExistsForUnreadState", "_sessionListLoaded", + "_sessionViewedCountRecord", + "_sessionViewedCountValue", + "_sessionViewedRecordWins", + "_sessionTranscriptGenerationForUnread", "_mergeSessionViewedCounts", "_sessionViewedCountDeletedKey", "_readSessionViewedCountDeletions", @@ -161,6 +166,12 @@ let _allSessions = []; const _sessionListSnapshotById = new Map(); const listed = (...ids) => {{ _allSessions = ids.map((sid) => ({{session_id: sid}})); }}; const readDisk = (key) => JSON.parse(_store[key] || '{{}}'); +const readViewedDisk = () => Object.fromEntries( + Object.entries(readDisk(SESSION_VIEWED_COUNTS_KEY)).map(([sid, value]) => [ + sid, + value && typeof value === 'object' ? Number(value.message_count) || 0 : Number(value) || 0, + ]) +); const setDisk = (key, value) => {{ _store[key] = JSON.stringify(value); }}; // Aggregate either the legacy whole map or independent per-session records. const readAllClears = (client) => client && typeof client.clears === 'function' @@ -189,6 +200,7 @@ def _client_factory(with_listener: bool = False) -> str: function makeClient() {{ let _sessionViewedCounts = null; let _sessionCompletionUnread = null; + let _sessionCompletionUnreadClearedMemory = {{}}; {fns} {helpers} {handler} @@ -196,7 +208,7 @@ function makeClient() {{ return {{ viewed: (sid) => {{ const counts = _getSessionViewedCounts(); - return Object.prototype.hasOwnProperty.call(counts, sid) ? counts[sid] : null; + return Object.prototype.hasOwnProperty.call(counts, sid) ? _sessionViewedCountValue(counts[sid]) : null; }}, setViewed: _setSessionViewedCount, clearViewed: _clearSessionViewedCount, @@ -204,6 +216,12 @@ function makeClient() {{ clearUnread: _clearSessionCompletionUnread, writeClear: _writeSessionCompletionUnreadCleared, hasUnread: _hasSessionCompletionUnread, + hasUnreadFor: (sid, messageCount, transcriptGeneration = 0, transcriptGenerationBaseline = 0) => _hasUnreadForSession({{ + session_id: sid, + message_count: messageCount, + transcript_generation: transcriptGeneration, + transcript_generation_baseline: transcriptGenerationBaseline, + }}), clears: (typeof _readSessionCompletionUnreadCleared === 'function') ? _readSessionCompletionUnreadCleared : () => readDisk(SESSION_COMPLETION_UNREAD_CLEARED_KEY), @@ -222,7 +240,92 @@ def _script(body: str, with_listener: bool = False) -> str: return _HEADER + _client_factory(with_listener) + body -# ── Viewed counts: monotonic, repaired, never resurrected ──────────────────── +# ── Viewed counts: generation-scoped, repaired, never resurrected ───────────── + +def test_shrink_then_growth_is_unread_in_the_new_transcript_generation(): + """A pre-shrink high-water mark must not mask new messages after truncation.""" + out = _run_node(_script(""" +listed('X'); +const A = makeClient(); +A.setViewed('X', 10, 0); +A.setViewed('X', 2, 1); +console.log(JSON.stringify({ + afterShrink: A.viewed('X'), + unreadAtThree: A.hasUnreadFor('X', 3, 1), +})); +""")) + assert out["afterShrink"] == 2 + assert out["unreadAtThree"] is True + + +def test_first_observation_of_new_generation_keeps_post_shrink_growth_unread(): + """A coalesced shrink-plus-growth refresh must acknowledge only the shrink baseline.""" + out = _run_node(_script(""" +listed('X'); +const A = makeClient(); +A.setViewed('X', 10, 0); +console.log(JSON.stringify({ + unread: A.hasUnreadFor('X', 3, 1, 2), + viewed: A.viewed('X'), +})); +""")) + assert out == {"unread": True, "viewed": 2} + + +def test_zero_message_baseline_survives_persistence_and_marks_first_message_unread(): + """Presence of a zero baseline is meaningful; it is not a missing record.""" + out = _run_node(_script(""" +listed('X'); +const A = makeClient(); +A.setViewed('X', 0, 0); +console.log(JSON.stringify({ + diskHasX: Object.prototype.hasOwnProperty.call(readViewedDisk(), 'X'), + unreadAtOne: A.hasUnreadFor('X', 1, 0), +})); +""")) + assert out["diskHasX"] is True + assert out["unreadAtOne"] is True + + +def test_in_memory_clear_expiry_uses_recorded_time_not_future_logical_order(): + """A future logical stamp must not extend the seven-day memory lifetime.""" + out = _run_node(_script(""" +const A = makeClient(); +_now = 1000; +A.writeClear('X', 999999999999); +const initially = A.clears(); +_now += 8 * 24 * 60 * 60 * 1000; +A.writeClear('Y', 5); +const expired = A.clears(); +console.log(JSON.stringify({initially, expired})); +""")) + assert out["initially"]["X"] == 999999999999 + assert "X" not in out["expired"] + + +def test_unread_clear_survives_quota_rejecting_a_new_tombstone_key(): + """Clearing must still shrink the existing marker map when allocation fails.""" + out = _run_node(_script(""" +listed('X'); +setDisk(SESSION_COMPLETION_UNREAD_KEY, { + X: {message_count: 3, completed_at: 1000, unread_order: 1}, +}); +const A = makeClient(); +const before = A.hasUnread('X'); +const ordinarySetItem = localStorage.setItem; +localStorage.setItem = (key, value) => { + if (!Object.prototype.hasOwnProperty.call(_store, key)) throw new Error('QuotaExceededError'); + ordinarySetItem(key, value); +}; +A.clearUnread('X'); +console.log(JSON.stringify({ + before, + after: A.hasUnread('X'), + diskHasX: Object.prototype.hasOwnProperty.call(readDisk(SESSION_COMPLETION_UNREAD_KEY), 'X'), +})); +""")) + assert out == {"before": True, "after": False, "diskHasX": False} + def test_stale_client_cannot_roll_a_viewed_count_back(): """Sequential rollback: a client that saw an older list snapshot must not @@ -234,7 +337,7 @@ const B = makeClient(); A.setViewed('X', 417); B.setViewed('X', 131); console.log(JSON.stringify({ - disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + disk: readViewedDisk(), bView: B.viewed('X'), })); """)) @@ -252,7 +355,7 @@ const A = makeClient(); A.setViewed('D', 5); A.clearViewed('D'); console.log(JSON.stringify({ - diskHasD: Object.prototype.hasOwnProperty.call(readDisk(SESSION_VIEWED_COUNTS_KEY), 'D'), + diskHasD: Object.prototype.hasOwnProperty.call(readViewedDisk(), 'D'), })); """)) assert out["diskHasD"] is False, "clearing a viewed count must remove it from disk" @@ -275,7 +378,7 @@ const bCached = B.viewed('E'); B.clearViewed('E'); console.log(JSON.stringify({ bCached: bCached, - disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + disk: readViewedDisk(), })); """)) assert out["bCached"] is None, "precondition: B's cache must not hold E" @@ -302,7 +405,7 @@ const held = A.viewed('X'); A.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); console.log(JSON.stringify({ held: held, - disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + disk: readViewedDisk(), })); """)) assert out["held"] == 417, ( @@ -329,10 +432,10 @@ B.viewed('Y'); // so A's write (built from the store it read) drops B's entry. _onRead = () => { B.setViewed('Y', 999); }; A.setViewed('X', 417); -const clobbered = readDisk(SESSION_VIEWED_COUNTS_KEY); +const clobbered = readViewedDisk(); // B still holds its own acknowledgement, and now processes A's storage event. B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); -const disk = readDisk(SESSION_VIEWED_COUNTS_KEY); +const disk = readViewedDisk(); const writesAfterRepair = storageWrites; B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); console.log(JSON.stringify({ @@ -372,7 +475,7 @@ listed('T'); B.setViewed('T', 1); B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); console.log(JSON.stringify({ - disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + disk: readViewedDisk(), tView: B.viewed('T'), })); """)) @@ -399,7 +502,7 @@ A.clearViewed('S'); B.handleStorage({ key: SESSION_VIEWED_COUNTS_KEY }); B.setViewed('T', 1); console.log(JSON.stringify({ - disk: readDisk(SESSION_VIEWED_COUNTS_KEY), + disk: readViewedDisk(), deletions: readViewedDeletions().length, bView: B.viewed('S'), })); @@ -777,7 +880,7 @@ const disk = readDisk(SESSION_COMPLETION_UNREAD_KEY); console.log(JSON.stringify({ handlers: _storageHandlers.length, xView: A.viewed('X'), - diskX: readDisk(SESSION_VIEWED_COUNTS_KEY).X, + diskX: readViewedDisk().X, stable: repairStable, qStale: qStale, qView: A.hasUnread('Q'), diff --git a/tests/test_webui_run_state_consistency_contract_rfc.py b/tests/test_webui_run_state_consistency_contract_rfc.py index b8c38a10..dedee864 100644 --- a/tests/test_webui_run_state_consistency_contract_rfc.py +++ b/tests/test_webui_run_state_consistency_contract_rfc.py @@ -46,6 +46,15 @@ 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( From cdcd37f98659ede75d3a9dccd161c2f1dc503a28 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Thu, 17 Sep 2026 14:05:38 +0300 Subject: [PATCH 13/16] fix: retain durable unread clears under storage pressure Keep an older persisted unread-clear ordering record when quota rejects its higher replacement, so reload cannot lose the last durable tombstone. Replace unread source-shape assertions with Node-driven behavior checks against the real sessions.js functions. --- .../webui-run-state-consistency-contract.md | 19 ++- static/sessions.js | 46 +++--- tests/_unread_store_helpers.py | 1 + ...t_issue856_background_completion_unread.py | 146 ++++++++++++++---- tests/test_unread_cross_client_sync.py | 31 ++++ ...ebui_run_state_consistency_contract_rfc.py | 9 -- 6 files changed, 185 insertions(+), 67 deletions(-) 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( From a4a87a97a3e0a2d5540620989e85fa879a4fed43 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Thu, 17 Sep 2026 14:28:11 +0300 Subject: [PATCH 14/16] test: prove first historical render creates no unread state The historical-session behavior check only asserted the first return value and the viewed baseline, so a _hasUnreadForSession regression that fabricated a completion-unread marker while returning false would pass and surface as unread on the next render. Also assert the second call stays false and that neither the in-memory completion-unread cache nor the persisted store gains a marker for the session. --- ...t_issue856_background_completion_unread.py | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/test_issue856_background_completion_unread.py b/tests/test_issue856_background_completion_unread.py index 83dcc58a..f21bc03e 100644 --- a/tests/test_issue856_background_completion_unread.py +++ b/tests/test_issue856_background_completion_unread.py @@ -619,15 +619,31 @@ def test_completion_unread_clears_only_when_session_is_opened(): 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({ +const session = { session_id: 'X', message_count: 5, transcript_generation: 3, transcript_generation_baseline: 0, -}); +}; +const first = _hasUnreadForSession(session); +// A first render that fabricates a completion-unread marker while still +// returning false would surface as unread on the next render (in-memory +// cache) or after a reload (persisted store), so inspect both before the +// second call. +const cachedMarker = Boolean( + _sessionCompletionUnread && + Object.prototype.hasOwnProperty.call(_sessionCompletionUnread, 'X') +); +const persistedMarker = Object.prototype.hasOwnProperty.call( + JSON.parse(store[SESSION_COMPLETION_UNREAD_KEY] || '{}'), 'X' +); +const second = _hasUnreadForSession(session); const viewed = JSON.parse(store[SESSION_VIEWED_COUNTS_KEY]).X; -console.log(JSON.stringify({unread, viewed})); +console.log(JSON.stringify({first, second, cachedMarker, persistedMarker, viewed})); """) assert out == { - "unread": False, + "first": False, + "second": False, + "cachedMarker": False, + "persistedMarker": False, "viewed": {"message_count": 5, "transcript_generation": 3}, } From df35836b75d46d819984fd051ff9113983183191 Mon Sep 17 00:00:00 2001 From: Rocky Date: Thu, 24 Sep 2026 18:01:04 +0300 Subject: [PATCH 15/16] fix: preserve transcript generations in sidebar projection Rocky-Actor: Hermes Rocky Rocky-Mode: interactive --- api/route_session_list_cache.py | 2 + .../webui-run-state-consistency-contract.md | 4 +- tests/test_sidebar_generation_projection.py | 48 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 tests/test_sidebar_generation_projection.py diff --git a/api/route_session_list_cache.py b/api/route_session_list_cache.py index b91ba7b8..41ea7d22 100644 --- a/api/route_session_list_cache.py +++ b/api/route_session_list_cache.py @@ -53,6 +53,8 @@ _SIDEBAR_SESSION_RESPONSE_FIELDS = { "model", "model_provider", "message_count", + "transcript_generation", + "transcript_generation_baseline", "user_message_count", "created_at", "updated_at", diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index 408fd3e0..78cc89bf 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -171,7 +171,9 @@ writes. - **Viewed counts are generation-scoped.** Session mutation routes increment the persisted `transcript_generation` whenever edit, regenerate, retry, undo, clear, or truncate reduces the visible transcript, and record the retained count as - `transcript_generation_baseline`. A newer generation replaces an older one even + `transcript_generation_baseline`. Both fields survive the bounded `/api/sessions` + projection for visible and sidebar-reference rows, including cached responses. + A newer generation replaces an older one even when its count is lower; counts are monotonic only within one generation and merge by maximum there. A client first observing a newer generation acknowledges only that retained baseline, so messages added after the shrink remain unread diff --git a/tests/test_sidebar_generation_projection.py b/tests/test_sidebar_generation_projection.py new file mode 100644 index 00000000..2454481c --- /dev/null +++ b/tests/test_sidebar_generation_projection.py @@ -0,0 +1,48 @@ +"""The sessions route must carry shrink metadata through to unread decisions.""" +import json +import shutil + +import pytest + +from tests.test_issue4766_sidebar_source_pushdown import ( + _handle_sessions, + _install_common_monkeypatches, +) +from tests.test_issue856_background_completion_unread import _run_unread_behavior + + +@pytest.mark.skipif(shutil.which("node") is None, reason="node not on PATH") +@pytest.mark.parametrize("retained", [0, 2]) +def test_sessions_route_preserves_post_shrink_unread(monkeypatch, retained): + import api.routes as routes + from api.models import Session + from api.session_ops import truncate_session_at_keep + + session = Session(session_id="X", messages=[ + {"role": "user", "content": str(i)} for i in range(10) + ]) + truncate_session_at_keep(session, retained) + session.messages.append({"role": "assistant", "content": "new response"}) + row = session.compact() + _install_common_monkeypatches(monkeypatch, [row]) + routes._session_list_cache_clear() + try: + # Exercise both the initial projection and the cached response. + for _ in range(2): + response = _handle_sessions("http://example.com/api/sessions?sidebar_source=webui") + assert response.status == 200 + [projected] = response.json_body()["sessions"] + result = _run_unread_behavior(""" +store[SESSION_VIEWED_COUNTS_KEY] = JSON.stringify({ + X: {message_count: 10, transcript_generation: 0}, +}); +const unread = _hasUnreadForSession(""" + json.dumps(projected) + """); +console.log(JSON.stringify({unread, viewed: JSON.parse(store[SESSION_VIEWED_COUNTS_KEY]).X})); +""") + assert result == { + "unread": True, + "viewed": {"message_count": retained, "transcript_generation": 1}, + } + assert "messages" not in projected + finally: + routes._session_list_cache_clear() From 89dbaec6f57b145f81a00984abadb384a44c79a5 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Thu, 24 Sep 2026 19:11:31 +0300 Subject: [PATCH 16/16] test: exercise compact parent projection --- tests/test_465_session_branching.py | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/tests/test_465_session_branching.py b/tests/test_465_session_branching.py index 2d8f1e8a..c59021d7 100644 --- a/tests/test_465_session_branching.py +++ b/tests/test_465_session_branching.py @@ -599,20 +599,12 @@ def test_session_model_parent_session_id(): def test_session_compact_includes_parent(): - """Verify compact() includes parent_session_id.""" - src = _read('api/models.py') - # Find the compact method and scan its full body for parent_session_id. - # PR #1591 (May 2026) added a has_pending_user_message recompute block at - # the top of compact() which pushed the parent_session_id field beyond a - # 1500-char window — widen the scan to 3000 chars to cover the full - # return-dict body without re-tightening every time compact() grows. - # compact() accepts optional projection flags on separate lines; match the - # method name and first parameter without pinning formatting. - compact_def_match = re.search(r"def compact\(\s*self", src) - assert compact_def_match, "Could not find compact() method" - snippet = src[compact_def_match.start():compact_def_match.start() + 3500] - assert "'parent_session_id'" in snippet, \ - "compact() should include parent_session_id" + """Verify compact() exposes a fork's parent and omits it for root sessions.""" + fork = routes.Session(session_id="fork-1", parent_session_id="parent-1") + assert fork.compact()["parent_session_id"] == "parent-1" + + root = routes.Session(session_id="root-1") + assert "parent_session_id" not in root.compact() def test_session_metadata_fields_includes_parent():