From 9b81ac4d482ac05cb9e19f28e985c52835244902 Mon Sep 17 00:00:00 2001 From: Michael Snoyman Date: Tue, 15 Sep 2026 16:53:29 +0300 Subject: [PATCH] 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,