From bc225657e98bb638295278f3f3db713afbb8d2ea Mon Sep 17 00:00:00 2001 From: Tolga Cinisli Date: Wed, 12 Aug 2026 14:59:31 +0300 Subject: [PATCH 1/2] fix(desktop): rank read-marker eviction by read recency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marking an older message read is undone by the very write that records it. `pruneStaleContexts` and `trimContextsToBudget` both rank `msg:`/`thread:` markers by the marker value — the timestamp of the *message* that was read — so a marker created just now for an older message sorts below the 7-day horizon or below the 1000-entry cap and is discarded before it reaches disk or the relay. The Inbox row reads correctly for the rest of the session and is unread again after the next reload, permanently. The horizon alone reproduces this at any volume: no message older than 7 days can be durably marked read. Rank both eviction points by when the read happened, using the `contextSourceCreatedAt` signal the manager already persists. That signal first has to mean what its name says: `applyRemoteContextTimestamp` refreshed it for every context in every republished blob, even when nothing advanced, collapsing it to "time of last publish". It now moves only on an actual advance, matching the publish path, which already bumps changed keys only. The 7-day horizon now measures time since the read rather than the age of the message read — same retention window, correct anchor. No storage format or wire change; contexts with no recency recorded behave exactly as before. `trimContextsToBudget` and `splitContextsIntoBudgetedSlots` move verbatim into `readStateBudget.ts`: `readStateManager.ts` sat at 999 of the 1000-line ceiling enforced by check-file-sizes, so the fix could not land in it. Both are pure functions already exported only for unit testing, and the eviction policy now sits beside the recency helper it ranks by. Signed-off-by: Tolga Cinisli --- .../channels/readState/readStateBudget.ts | 189 ++++++++++++++++++ .../channels/readState/readStateFormat.ts | 18 ++ .../readState/readStateManager.test.mjs | 65 +++++- .../channels/readState/readStateManager.ts | 177 ++-------------- .../readState/readStateStorage.test.mjs | 68 +++++++ .../channels/readState/readStateStorage.ts | 45 ++++- 6 files changed, 386 insertions(+), 176 deletions(-) create mode 100644 desktop/src/features/channels/readState/readStateBudget.ts diff --git a/desktop/src/features/channels/readState/readStateBudget.ts b/desktop/src/features/channels/readState/readStateBudget.ts new file mode 100644 index 00000000000..fdbcfa1e326 --- /dev/null +++ b/desktop/src/features/channels/readState/readStateBudget.ts @@ -0,0 +1,189 @@ +/** + * Byte-budget eviction for NIP-RS read-state blobs. + * + * Pure helpers, split out of `readStateManager` so the eviction policy lives + * next to the recency signal it ranks by. Callers should prefer the manager's + * `currentContexts()` / `splitContextsIntoSlots()`; these are exported for + * direct unit testing. + */ +import { + MSG_PREFIX, + THREAD_PREFIX, + readActionRecency, +} from "@/features/channels/readState/readStateFormat"; + +/** + * Result of a `splitContextsIntoBudgetedSlots` call. + */ +export interface SlotSplitResult { + /** Contexts record for each slot (primary slot first). */ + slots: Array>; + /** + * Extra slot IDs allocated beyond the first. Length is `slots.length - 1`. + * The caller is responsible for persisting these. + */ + extraSlotIds: string[]; +} + +/** + * Partition `channelEntries` across slots so each slot's blob fits within + * `maxBytes`. Thread/msg entries are added to the primary slot (index 0) and + * trimmed to budget. + * + * `initialSlotCount` is the number of slots already available (≥ 1). If the + * initial distribution doesn't fit, new slot IDs are generated via + * `slotIdGenerator` until everything fits or `maxSlots` is reached. + * + * Returns `{ slots, extraSlotIds }` on success, or `null` when even `maxSlots` + * slots can't accommodate all channel keys. + * + * Exported for unit testing; callers should prefer `splitContextsIntoSlots()`. + */ +export function splitContextsIntoBudgetedSlots(args: { + channelEntries: [string, number][]; + threadMsgEntries: [string, number][]; + clientId: string; + initialSlotCount: number; + maxSlots: number; + maxBytes: number; + slotIdGenerator: () => string; + contextSourceCreatedAt?: ReadonlyMap; +}): SlotSplitResult | null { + const { + channelEntries, + threadMsgEntries, + clientId, + initialSlotCount, + maxSlots, + maxBytes, + slotIdGenerator, + contextSourceCreatedAt, + } = args; + + const encoder = new TextEncoder(); + const blobFor = (c: Record) => + JSON.stringify({ v: 1, client_id: clientId, contexts: c }); + + let slotCount = initialSlotCount; + const extraSlotIds: string[] = []; + + // Distribute channel keys and check fit. Grow slot count until all fit. + const distribute = (count: number): Array> => { + const slotContexts: Array> = Array.from( + { length: count }, + () => ({}), + ); + for (let i = 0; i < channelEntries.length; i++) { + const [key, ts] = channelEntries[i]; + slotContexts[i % count][key] = ts; + } + return slotContexts; + }; + + let slotContexts = distribute(slotCount); + while ( + slotContexts.some((c) => encoder.encode(blobFor(c)).length > maxBytes) && + slotCount < maxSlots + ) { + extraSlotIds.push(slotIdGenerator()); + slotCount++; + slotContexts = distribute(slotCount); + } + + if (slotContexts.some((c) => encoder.encode(blobFor(c)).length > maxBytes)) { + return null; + } + + // Add thread/msg entries to the primary slot and trim to budget. + for (const [key, ts] of threadMsgEntries) { + slotContexts[0][key] = ts; + } + trimContextsToBudget( + slotContexts[0], + clientId, + maxBytes, + contextSourceCreatedAt, + ); + + return { slots: slotContexts, extraSlotIds }; +} + +/** + * Result of a `trimContextsToBudget` call. + */ +export interface TrimResult { + /** Number of entries removed from `contexts`. */ + evicted: number; + /** True when the serialized blob fits within `maxBytes` after trimming. */ + fitsAfterTrim: boolean; +} + +/** + * Trim a contexts map to fit within `maxBytes` when serialized as the JSON + * blob `{v:1, client_id, contexts}`. Evicts least-recently-read `msg:` entries + * first, then least-recently-read `thread:` entries. Channel keys are never + * evicted. Mutates `contexts` in place. + * + * Recency comes from `contextSourceCreatedAt` (see `readActionRecency`) and + * falls back to the marker value. Ranking by the marker value alone evicts a + * marker the user just created on an older message ahead of markers they last + * touched days ago, so the read never survives the publish. + * + * Returns `{ evicted, fitsAfterTrim }`. `fitsAfterTrim` is false when the + * remaining blob (channel keys only) still exceeds `maxBytes` — the caller + * must not publish in that case. + * + * Exported for unit testing; callers should prefer `currentContexts()`. + */ +export function trimContextsToBudget( + contexts: Record, + clientId: string, + maxBytes: number, + contextSourceCreatedAt?: ReadonlyMap, +): TrimResult { + const encoder = new TextEncoder(); + const blobFor = (c: Record) => + JSON.stringify({ v: 1, client_id: clientId, contexts: c }); + + let currentBytes = encoder.encode(blobFor(contexts)).length; + if (currentBytes <= maxBytes) { + return { evicted: 0, fitsAfterTrim: true }; + } + + const msgEntries: [string, number][] = []; + const threadEntries: [string, number][] = []; + for (const [key, ts] of Object.entries(contexts)) { + if (key.startsWith(MSG_PREFIX)) { + msgEntries.push([key, ts]); + } else if (key.startsWith(THREAD_PREFIX)) { + threadEntries.push([key, ts]); + } + } + // Least-recently-read first within each tier. + const byReadRecency = (a: [string, number], b: [string, number]) => + readActionRecency(a[0], a[1], contextSourceCreatedAt) - + readActionRecency(b[0], b[1], contextSourceCreatedAt); + msgEntries.sort(byReadRecency); + threadEntries.sort(byReadRecency); + + // O(n) pass: subtract each entry's byte contribution from currentBytes and + // collect entries to evict. The per-entry estimate is `,"key":timestamp` + // (key.length + 3 bytes for `"`, `"`, `:` plus 1 comma) + timestamp digits. + // This is an approximation — the final encode below is the authoritative check. + const toEvict: string[] = []; + for (const [key, ts] of [...msgEntries, ...threadEntries]) { + if (currentBytes <= maxBytes) break; + // Contribution: `,"key":timestamp` — comma + quoted key + colon + value + currentBytes -= key.length + 3 + String(ts).length + 1; + toEvict.push(key); + } + + for (const key of toEvict) { + delete contexts[key]; + } + + // Final authoritative check — handles JSON comma-accounting edge cases + // (e.g. last-entry comma disappears) that the per-entry estimate ignores. + const fitsAfterTrim = encoder.encode(blobFor(contexts)).length <= maxBytes; + return { evicted: toEvict.length, fitsAfterTrim }; +} diff --git a/desktop/src/features/channels/readState/readStateFormat.ts b/desktop/src/features/channels/readState/readStateFormat.ts index 4cd61e9f43e..c5dc6f4c691 100644 --- a/desktop/src/features/channels/readState/readStateFormat.ts +++ b/desktop/src/features/channels/readState/readStateFormat.ts @@ -37,6 +37,24 @@ export const THREAD_PREFIX = "thread:"; const EVENT_ID_PATTERN = /^[0-9a-f]{64}$/; +/** + * Recency of the READ ACTION behind a marker — when this read fact entered the + * client — falling back to the marker value when unknown (contexts seeded + * before this signal was recorded). + * + * Eviction must rank by this, never by the marker timestamp. The marker value + * is the age of the *message* that was read, so ranking by it discards a marker + * the user just created on an older message while keeping long-stale markers + * that happen to point at recent messages. + */ +export function readActionRecency( + contextId: string, + markerTimestamp: number, + contextSourceCreatedAt?: ReadonlyMap, +): number { + return contextSourceCreatedAt?.get(contextId) ?? markerTimestamp; +} + export function maxReadAt(...markers: Array): number | null { return markers.reduce((latest, marker) => { if (marker === null) return latest; diff --git a/desktop/src/features/channels/readState/readStateManager.test.mjs b/desktop/src/features/channels/readState/readStateManager.test.mjs index 8831a952bf6..43f850f9e6b 100644 --- a/desktop/src/features/channels/readState/readStateManager.test.mjs +++ b/desktop/src/features/channels/readState/readStateManager.test.mjs @@ -5,9 +5,11 @@ import { ReadStateManager, applyRemoteContextTimestamp, resolveEffectiveTimestamp, +} from "./readStateManager.ts"; +import { splitContextsIntoBudgetedSlots, trimContextsToBudget, -} from "./readStateManager.ts"; +} from "./readStateBudget.ts"; // ── ReadStateManager integration helpers ───────────────────────────────────── // Provide browser globals required by ReadStateManager (localStorage, @@ -414,7 +416,28 @@ test("applyRemoteContextTimestamp ignores older remote read markers from newer s assert.equal(result, "unchanged"); assert.equal(effectiveState.get("channel-1"), 200); - assert.equal(contextSourceCreatedAt.get("channel-1"), 11); + // Recency tracks the last read ADVANCE, not the last blob that mentioned the + // context — a routine republish carrying nothing new must not refresh it. + assert.equal(contextSourceCreatedAt.get("channel-1"), 10); +}); + +test("applyRemoteContextTimestamp keeps recency stable across repeated republishes", () => { + const effectiveState = new Map([["channel-1", 200]]); + const contextSourceCreatedAt = new Map([["channel-1", 10]]); + + for (const eventCreatedAt of [50, 60, 70]) { + applyRemoteContextTimestamp({ + effectiveState, + contextSourceCreatedAt, + contextId: "channel-1", + timestamp: 200, + eventCreatedAt, + }); + } + + // Without this, every context in every republished blob would look freshly + // read and recency-ranked eviction would degenerate to publish order. + assert.equal(contextSourceCreatedAt.get("channel-1"), 10); }); test("applyRemoteContextTimestamp advances to newer remote read markers", () => { @@ -506,6 +529,44 @@ test("trimContextsToBudget_overBudget_evictsMsgEntriesOldestFirst", () => { ); }); +test("trimContextsToBudget_evictsLeastRecentlyRead_notOldestMessage", () => { + // msg A points at the OLDEST message but was read just now; msg B and C point + // at newer messages read long ago. Ranking by marker value would evict A — + // the read the user just performed. + const justReadOldMessage = `msg:${MSG_ID}`; + const contexts = { + [justReadOldMessage]: 1, + [`msg:${"c".repeat(64)}`]: 3, + [`msg:${"d".repeat(64)}`]: 2, + }; + const readRecency = new Map([ + [justReadOldMessage, 9_000], + [`msg:${"c".repeat(64)}`, 10], + [`msg:${"d".repeat(64)}`, 20], + ]); + const encoder = new TextEncoder(); + const budget = + encoder.encode(JSON.stringify({ v: 1, client_id: CLIENT_ID, contexts })) + .length - 10; + + const { fitsAfterTrim } = trimContextsToBudget( + contexts, + CLIENT_ID, + budget, + readRecency, + ); + + assert.equal(fitsAfterTrim, true); + assert.ok( + justReadOldMessage in contexts, + "the marker read most recently must survive", + ); + assert.ok( + !(`msg:${"c".repeat(64)}` in contexts), + "the least recently read marker should be evicted", + ); +}); + test("trimContextsToBudget_channelKeysNeverEvicted", () => { // Fill with msg entries plus one channel key; budget forces eviction. const contexts = {}; diff --git a/desktop/src/features/channels/readState/readStateManager.ts b/desktop/src/features/channels/readState/readStateManager.ts index 3ba382dc61e..eddd2893e3d 100644 --- a/desktop/src/features/channels/readState/readStateManager.ts +++ b/desktop/src/features/channels/readState/readStateManager.ts @@ -13,6 +13,10 @@ import { localExtraSlotIdsKey, type ReadStateBlob, } from "@/features/channels/readState/readStateFormat"; +import { + splitContextsIntoBudgetedSlots, + trimContextsToBudget, +} from "@/features/channels/readState/readStateBudget"; import { parseReadStateEvent } from "@/features/channels/readState/readStateSnapshot"; import { readStoredReadState, @@ -136,171 +140,16 @@ export function applyRemoteContextTimestamp(args: { if (result === "advanced") { effectiveState.set(contextId, next); - } - if (eventCreatedAt > sourceCreatedAt) { - contextSourceCreatedAt.set(contextId, eventCreatedAt); - } - return result; -} - -/** - * Result of a `splitContextsIntoBudgetedSlots` call. - */ -export interface SlotSplitResult { - /** Contexts record for each slot (primary slot first). */ - slots: Array>; - /** - * Extra slot IDs allocated beyond the first. Length is `slots.length - 1`. - * The caller is responsible for persisting these. - */ - extraSlotIds: string[]; -} - -/** - * Partition `channelEntries` across slots so each slot's blob fits within - * `maxBytes`. Thread/msg entries are added to the primary slot (index 0) and - * trimmed to budget. - * - * `initialSlotCount` is the number of slots already available (≥ 1). If the - * initial distribution doesn't fit, new slot IDs are generated via - * `slotIdGenerator` until everything fits or `maxSlots` is reached. - * - * Returns `{ slots, extraSlotIds }` on success, or `null` when even `maxSlots` - * slots can't accommodate all channel keys. - * - * Exported for unit testing; callers should prefer `splitContextsIntoSlots()`. - */ -export function splitContextsIntoBudgetedSlots(args: { - channelEntries: [string, number][]; - threadMsgEntries: [string, number][]; - clientId: string; - initialSlotCount: number; - maxSlots: number; - maxBytes: number; - slotIdGenerator: () => string; -}): SlotSplitResult | null { - const { - channelEntries, - threadMsgEntries, - clientId, - initialSlotCount, - maxSlots, - maxBytes, - slotIdGenerator, - } = args; - - const encoder = new TextEncoder(); - const blobFor = (c: Record) => - JSON.stringify({ v: 1, client_id: clientId, contexts: c }); - - let slotCount = initialSlotCount; - const extraSlotIds: string[] = []; - - // Distribute channel keys and check fit. Grow slot count until all fit. - const distribute = (count: number): Array> => { - const slotContexts: Array> = Array.from( - { length: count }, - () => ({}), - ); - for (let i = 0; i < channelEntries.length; i++) { - const [key, ts] = channelEntries[i]; - slotContexts[i % count][key] = ts; - } - return slotContexts; - }; - - let slotContexts = distribute(slotCount); - while ( - slotContexts.some((c) => encoder.encode(blobFor(c)).length > maxBytes) && - slotCount < maxSlots - ) { - extraSlotIds.push(slotIdGenerator()); - slotCount++; - slotContexts = distribute(slotCount); - } - - if (slotContexts.some((c) => encoder.encode(blobFor(c)).length > maxBytes)) { - return null; - } - - // Add thread/msg entries to the primary slot and trim to budget. - for (const [key, ts] of threadMsgEntries) { - slotContexts[0][key] = ts; - } - trimContextsToBudget(slotContexts[0], clientId, maxBytes); - - return { slots: slotContexts, extraSlotIds }; -} - -/** - * Result of a `trimContextsToBudget` call. - */ -export interface TrimResult { - /** Number of entries removed from `contexts`. */ - evicted: number; - /** True when the serialized blob fits within `maxBytes` after trimming. */ - fitsAfterTrim: boolean; -} - -/** - * Trim a contexts map to fit within `maxBytes` when serialized as the JSON - * blob `{v:1, client_id, contexts}`. Evicts oldest `msg:` entries first - * (lowest timestamp), then oldest `thread:` entries. Channel keys are never - * evicted. Mutates `contexts` in place. - * - * Returns `{ evicted, fitsAfterTrim }`. `fitsAfterTrim` is false when the - * remaining blob (channel keys only) still exceeds `maxBytes` — the caller - * must not publish in that case. - * - * Exported for unit testing; callers should prefer `currentContexts()`. - */ -export function trimContextsToBudget( - contexts: Record, - clientId: string, - maxBytes: number, -): TrimResult { - const encoder = new TextEncoder(); - const blobFor = (c: Record) => - JSON.stringify({ v: 1, client_id: clientId, contexts: c }); - - let currentBytes = encoder.encode(blobFor(contexts)).length; - if (currentBytes <= maxBytes) { - return { evicted: 0, fitsAfterTrim: true }; - } - - const msgEntries: [string, number][] = []; - const threadEntries: [string, number][] = []; - for (const [key, ts] of Object.entries(contexts)) { - if (key.startsWith(MSG_PREFIX)) { - msgEntries.push([key, ts]); - } else if (key.startsWith(THREAD_PREFIX)) { - threadEntries.push([key, ts]); + // Only an advance is a NEW read fact. Re-learning an unchanged context from + // a routine republish must not refresh its recency, or every context in + // every blob would look freshly read and `contextSourceCreatedAt` would + // collapse to "time of last publish" — useless as an eviction key. Mirrors + // the publish path, which already bumps only changed keys. + if (eventCreatedAt > sourceCreatedAt) { + contextSourceCreatedAt.set(contextId, eventCreatedAt); } } - // Oldest-first within each tier. - msgEntries.sort((a, b) => a[1] - b[1]); - threadEntries.sort((a, b) => a[1] - b[1]); - - // O(n) pass: subtract each entry's byte contribution from currentBytes and - // collect entries to evict. The per-entry estimate is `,"key":timestamp` - // (key.length + 3 bytes for `"`, `"`, `:` plus 1 comma) + timestamp digits. - // This is an approximation — the final encode below is the authoritative check. - const toEvict: string[] = []; - for (const [key, ts] of [...msgEntries, ...threadEntries]) { - if (currentBytes <= maxBytes) break; - // Contribution: `,"key":timestamp` — comma + quoted key + colon + value - currentBytes -= key.length + 3 + String(ts).length + 1; - toEvict.push(key); - } - - for (const key of toEvict) { - delete contexts[key]; - } - - // Final authoritative check — handles JSON comma-accounting edge cases - // (e.g. last-entry comma disappears) that the per-entry estimate ignores. - const fitsAfterTrim = encoder.encode(blobFor(contexts)).length <= maxBytes; - return { evicted: toEvict.length, fitsAfterTrim }; + return result; } export class ReadStateManager { @@ -855,6 +704,7 @@ export class ReadStateManager { contexts, this.clientId, READ_STATE_MAX_PLAINTEXT_BYTES, + this.contextSourceCreatedAt, ); if (evicted > 0) { console.warn( @@ -907,6 +757,7 @@ export class ReadStateManager { maxSlots: READ_STATE_MAX_SLOTS, maxBytes: READ_STATE_MAX_PLAINTEXT_BYTES, slotIdGenerator: () => generateHex(16), + contextSourceCreatedAt: this.contextSourceCreatedAt, }); if (result === null) { diff --git a/desktop/src/features/channels/readState/readStateStorage.test.mjs b/desktop/src/features/channels/readState/readStateStorage.test.mjs index ad7eb031ebc..43fc435b610 100644 --- a/desktop/src/features/channels/readState/readStateStorage.test.mjs +++ b/desktop/src/features/channels/readState/readStateStorage.test.mjs @@ -76,6 +76,74 @@ test("pruneStaleContexts caps within-horizon prunable entries, newest kept", () assert.equal(pruned.has(`msg:${String(total - 1).padStart(64, "0")}`), false); }); +test("pruneStaleContexts keeps a marker just read on an old message", () => { + // The user marks a 30-day-old message read today. The marker carries the + // MESSAGE's timestamp, so a value-ranked horizon drops it on the same write. + const oldMessage = `msg:${"e".repeat(64)}`; + const contexts = new Map([ + [oldMessage, NOW - READ_STATE_HORIZON_SECONDS * 4], + ]); + const readJustNow = new Map([[oldMessage, NOW]]); + + assert.equal( + pruneStaleContexts(contexts, NOW).has(oldMessage), + false, + "value-ranked horizon drops it (documents the old behavior)", + ); + assert.equal( + pruneStaleContexts(contexts, NOW, readJustNow).has(oldMessage), + true, + "read-recency horizon keeps it", + ); +}); + +test("pruneStaleContexts cap evicts least recently read, not oldest message", () => { + const contexts = new Map(); + const recency = new Map(); + const total = LOCAL_MAX_PRUNABLE_CONTEXTS + 1; + // Newest messages first (i=0 newest), all read a long time ago... + for (let i = 0; i < total - 1; i++) { + const key = `msg:${String(i).padStart(64, "0")}`; + contexts.set(key, NOW - i); + recency.set(key, NOW - READ_STATE_HORIZON_SECONDS / 2); + } + // ...and one old message read just now, which must survive the cap. + const justRead = `msg:${"f".repeat(64)}`; + contexts.set(justRead, NOW - READ_STATE_HORIZON_SECONDS + 60); + recency.set(justRead, NOW); + + const valueRanked = pruneStaleContexts(contexts, NOW); + assert.equal(valueRanked.size, LOCAL_MAX_PRUNABLE_CONTEXTS); + assert.equal( + valueRanked.has(justRead), + false, + "value-ranked cap evicts it (documents the old behavior)", + ); + + const recencyRanked = pruneStaleContexts(contexts, NOW, recency); + assert.equal(recencyRanked.size, LOCAL_MAX_PRUNABLE_CONTEXTS); + assert.equal(recencyRanked.has(justRead), true, "read-recency cap keeps it"); +}); + +test("writeStoredReadState keeps a marker just read on an old message", () => { + installLocalStorage(); + const pubkey = "c".repeat(64); + const nowSeconds = Math.floor(Date.now() / 1_000); + const oldMessage = `msg:${"a".repeat(64)}`; + + writeStoredReadState( + pubkey, + new Map([[oldMessage, nowSeconds - READ_STATE_HORIZON_SECONDS * 4]]), + new Set([oldMessage]), + new Map([[oldMessage, nowSeconds]]), + ); + + const state = JSON.parse( + window.localStorage.getItem(localReadStateKey(pubkey)), + ); + assert.deepEqual(Object.keys(state), [oldMessage]); +}); + test("writeStoredReadState prunes all three keys consistently", () => { installLocalStorage(); const pubkey = "f".repeat(64); diff --git a/desktop/src/features/channels/readState/readStateStorage.ts b/desktop/src/features/channels/readState/readStateStorage.ts index f5ac8996134..490d17793b3 100644 --- a/desktop/src/features/channels/readState/readStateStorage.ts +++ b/desktop/src/features/channels/readState/readStateStorage.ts @@ -8,6 +8,7 @@ import { MSG_PREFIX, READ_STATE_HORIZON_SECONDS, THREAD_PREFIX, + readActionRecency, } from "@/features/channels/readState/readStateFormat"; import { setLocalStorageItemWithRecovery } from "@/shared/lib/localStorageQuota"; @@ -110,33 +111,51 @@ function isPrunableContextKey(contextId: string): boolean { } /** - * Drops msg:/thread: markers older than the relay's 7-day horizon, then caps - * the survivors at LOCAL_MAX_PRUNABLE_CONTEXTS (oldest first). Channel keys - * are never pruned — they are small, bounded by membership, and losing one - * would resurrect the channel's unread badge. Mirrors the eviction order the - * publish path already applies in trimContextsToBudget. + * Drops msg:/thread: markers whose READ ACTION is older than the 7-day horizon, + * then caps the survivors at LOCAL_MAX_PRUNABLE_CONTEXTS (least recently read + * evicted first). Channel keys are never pruned — they are small, bounded by + * membership, and losing one would resurrect the channel's unread badge. + * Mirrors the eviction order the publish path applies in trimContextsToBudget. + * + * Both the horizon and the cap rank by `contextSourceCreatedAt` (see + * `readActionRecency`), NOT by the marker value. Ranking by the marker value + * means marking an older message read is undone by the very write that records + * it: the new marker carries that message's old timestamp, so it sorts below + * the cap — or below the horizon — and is dropped before it ever reaches disk. */ export function pruneStaleContexts( contexts: ReadonlyMap, nowUnixSeconds: number, + contextSourceCreatedAt?: ReadonlyMap, ): Map { const cutoff = nowUnixSeconds - READ_STATE_HORIZON_SECONDS; const kept = new Map(); - const prunable: [string, number][] = []; + const prunable: Array<{ + contextId: string; + timestamp: number; + recency: number; + }> = []; for (const [contextId, timestamp] of contexts) { if (!isPrunableContextKey(contextId)) { kept.set(contextId, timestamp); - } else if (timestamp >= cutoff) { - prunable.push([contextId, timestamp]); + continue; + } + const recency = readActionRecency( + contextId, + timestamp, + contextSourceCreatedAt, + ); + if (recency >= cutoff) { + prunable.push({ contextId, timestamp, recency }); } } if (prunable.length > LOCAL_MAX_PRUNABLE_CONTEXTS) { - prunable.sort((a, b) => b[1] - a[1]); + prunable.sort((a, b) => b.recency - a.recency); prunable.length = LOCAL_MAX_PRUNABLE_CONTEXTS; } - for (const [contextId, timestamp] of prunable) { + for (const { contextId, timestamp } of prunable) { kept.set(contextId, timestamp); } return kept; @@ -148,7 +167,11 @@ export function writeStoredReadState( publishableContextIds: ReadonlySet, contextSourceCreatedAt: ReadonlyMap, ): void { - const pruned = pruneStaleContexts(contexts, Math.floor(Date.now() / 1_000)); + const pruned = pruneStaleContexts( + contexts, + Math.floor(Date.now() / 1_000), + contextSourceCreatedAt, + ); const state: Record = {}; for (const [contextId, timestamp] of pruned) { From b9900dd6a1bd57b10d50a90927e2260e101b7407 Mon Sep 17 00:00:00 2001 From: Tolga Cinisli Date: Thu, 20 Aug 2026 23:21:25 +0300 Subject: [PATCH 2/2] fix(desktop): stop the publish path from restamping read recency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `publishOneSlot` stamped `contextSourceCreatedAt` for every key whose value differed from `lastPublishedContexts`. That guard is not the narrow filter it looks like: `initialize` seeds `lastPublishedContexts` from the union of the *relay* blobs, while local storage restores the full context set, so every key `trimContextsToBudget` dropped is missing from the relay copy, reads as changed on the first publish after each launch, and is restamped with a single `createdAt`. Measured on a live account (349 contexts, 330 of them prunable and all carrying a recency value): only three distinct values across the whole tier, all within half an hour of each other — publish-shaped, not read-shaped. With that distribution the 7-day horizon check evicts nothing, because every context looks freshly read, and at the cap the comparator sees ties, so eviction order falls back to sort stability instead of read recency. Recency is now written only where a read happens: `markContextRead` and the advance branch of `applyRemoteContextTimestamp`. No context loses its signal — locally marked ones are stamped by `markContextRead`, remotely learned ones by the merge advance, which is also the path blob-seeded contexts arrive through. Reported-by: Roberto Michelena Signed-off-by: Tolga Cinisli --- .../readState/readStateManager.test.mjs | 60 +++++++++++++++++++ .../channels/readState/readStateManager.ts | 18 +++--- 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/desktop/src/features/channels/readState/readStateManager.test.mjs b/desktop/src/features/channels/readState/readStateManager.test.mjs index 43f850f9e6b..121020e6c77 100644 --- a/desktop/src/features/channels/readState/readStateManager.test.mjs +++ b/desktop/src/features/channels/readState/readStateManager.test.mjs @@ -270,6 +270,66 @@ test("publish flushes pending local state first", async () => { } }); +test("publishing does not refresh read recency", async () => { + globalThis.window.localStorage = makeLocalStorage(); + const { restore } = withFakeTimers(); + const pubkey = "7".repeat(64); + const manager = new ReadStateManager(pubkey, makeFakeRelay()); + const msgKey = `msg:${"c".repeat(64)}`; + const markerValue = 1_000_000; // timestamp of the message that was read + const readHappenedAt = 1_500_000; // when that read entered the client + + // @tauri-apps/api/core reads `window.__TAURI_INTERNALS__.invoke`. + const originalInternals = globalThis.window.__TAURI_INTERNALS__; + const invoked = []; + globalThis.window.__TAURI_INTERNALS__ = { + invoke: (cmd, args) => { + invoked.push(cmd); + if (cmd === "nip44_encrypt_to_self") return Promise.resolve("ciphertext"); + if (cmd === "sign_event") { + return Promise.resolve( + JSON.stringify({ + id: "e".repeat(64), + pubkey, + created_at: args.createdAt, + kind: args.kind, + tags: args.tags, + content: args.content, + sig: "f".repeat(128), + }), + ); + } + return Promise.resolve(null); + }, + }; + + try { + manager.markContextRead(msgKey, markerValue); + manager.contextSourceCreatedAt.set(msgKey, readHappenedAt); + // Fresh launch on an account whose blob is trimmed: the relay copy that + // seeds lastPublishedContexts does not carry this key, so it reads as + // changed on the next publish. + manager.lastPublishedContexts = {}; + manager.fetchOwnBlobBeforePublish = async () => {}; + + await manager.publish(); + + assert.ok( + invoked.includes("sign_event"), + "precondition: the blob must actually publish", + ); + assert.equal( + manager.contextSourceCreatedAt.get(msgKey), + readHappenedAt, + "a publish is not a read: recency must stay at the time of the read", + ); + } finally { + globalThis.window.__TAURI_INTERNALS__ = originalInternals; + manager.destroy(); + restore(); + } +}); + test("destroyed manager cannot persist after an in-flight fetch resolves", async () => { const storage = makeLocalStorage(); globalThis.window.localStorage = storage; diff --git a/desktop/src/features/channels/readState/readStateManager.ts b/desktop/src/features/channels/readState/readStateManager.ts index eddd2893e3d..e7ebbea172d 100644 --- a/desktop/src/features/channels/readState/readStateManager.ts +++ b/desktop/src/features/channels/readState/readStateManager.ts @@ -143,8 +143,8 @@ export function applyRemoteContextTimestamp(args: { // Only an advance is a NEW read fact. Re-learning an unchanged context from // a routine republish must not refresh its recency, or every context in // every blob would look freshly read and `contextSourceCreatedAt` would - // collapse to "time of last publish" — useless as an eviction key. Mirrors - // the publish path, which already bumps only changed keys. + // collapse to "time of last publish" — useless as an eviction key. The + // publish path does not write recency at all, for the same reason. if (eventCreatedAt > sourceCreatedAt) { contextSourceCreatedAt.set(contextId, eventCreatedAt); } @@ -571,11 +571,15 @@ export class ReadStateManager { `[ReadStateManager] publish accepted slotId=${slotId} createdAt=${createdAt}`, ); - for (const key of Object.keys(contexts)) { - if (this.lastPublishedContexts[key] !== contexts[key]) { - this.contextSourceCreatedAt.set(key, createdAt); - } - } + // Publishing is not a read action, so it must not refresh + // `contextSourceCreatedAt`. Stamping "keys that changed since the last + // publish" is not the narrow filter it looks like: lastPublishedContexts + // is seeded from the *trimmed* relay blob, so every key + // trimContextsToBudget dropped reads as changed on the first publish + // after each launch, and the recency signal collapses to "time of last + // publish" for the whole prunable tier. Recency is written only where a + // read happens — markContextRead, and the advance branch of + // applyRemoteContextTimestamp. // Merge this slot's contexts into lastPublishedContexts (union). for (const [key, ts] of Object.entries(contexts)) { this.lastPublishedContexts[key] = ts;