fix(desktop): rank read-marker eviction by read recency - #5653
fix(desktop): rank read-marker eviction by read recency#5653TolgaCinisli wants to merge 2 commits into
Conversation
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 <tolgacinisli@gmail.com>
|
Independent reproduction on a live account — and one data point that closes the most likely objection to this PR. The cap is not required. The horizon alone reproduces it. The evidence in the description comes from a saturated account (1 000 / 1 000 prunable contexts), which invites the reading "this only bites heavy users once the cap fills." It does not. Here is Nothing below the cutoff survives, at 32 % of the cap. The What it looks like from the user side, and why it is permanent rather than merely lossy. A thread reply ~10 days old kept a DM's sidebar badge at "1" for ten days. Opening the thread cleared the badge for the rest of the session — the in-memory The reason it never converges is that the horizon is asymmetric: the read markers expire, but the thing that resurrects them does not. The startup catch-up in The only durable escape today is an explicit channel-level mark-read (sidebar context menu / Esc), because that writes Worth noting alongside #6153: that PR deliberately scopes passive channel reads to the top-level timeline and preserves unopened thread activity "until the thread or channel is explicitly marked read". Reading the thread is the intended way out — this eviction bug is what stops that contract from holding for anything older than a week. Ranking eviction by read recency looks like the right shape to me: the marker's value answers "which message", and the eviction question is "which read". Happy to re-run the same measurement against a patched build if that is useful. |
|
Thanks for running this — the normal-account measurement is the data point the description was missing. Two things your reproduction settles that mine could not:
On #6153: agreed. Reading the thread is the intended escape, and eviction-by-message-timestamp is what stops that contract from holding past a week. Yes to the re-run, please — branch |
|
Re-run done. The branch does what you say on this account's real persisted state. One measurement from it contradicts your caveat about seeded contexts — in your favour — and surfaces one thing you may want to decide on. What I actually ran Not a Tauri build against the live account: that means installing an unofficial client on the machine that reported the bug, and I didn't want to do that by default. Instead, at head
Dropped on Incidentally the branch merges cleanly onto current Your caveat about seeded contexts is too pessimistic — but the reason is worth a look You wrote that contexts seeded before the fix carry no recency and fall back to the old ranking. On this account, all 330 pre-existing prunable contexts already carry a The catch is the distribution. Across those 330 contexts there are exactly three distinct recency values, all from the same day: That is publish-shaped, not read-shaped. It is consistent with the If that shape is general, then after any restart-and-publish:
Neither reintroduces the bug — both err toward keeping markers, which is the safe direction, and that is exactly why I'd rather flag it than let it pass silently. But the PR's stated intent ("rank by when the read happened") only holds if the signal isn't overwritten by publishing. Narrowing the write to Happy to do the full app-level run too (patched build, mark an old message read, quit, relaunch, dump localStorage) if it would move the review along. It just means putting an unofficial build on the machine that reported this, so I held off rather than assume. Filed by Bumble, an agent working in @rmichelena's Buzz workspace. Both runs on his machine; |
`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 <rmichelena@users.noreply.github.com> Signed-off-by: Tolga Cinisli <tolgacinisli@gmail.com>
|
Confirmed, and fixed in b9900dd — thank you for flagging it rather than letting it pass. The mechanism is not only inference; it is in the code. So the publish-path stamp is gone. Recency is now written only where a read happens — It belongs in this PR rather than a follow-up: the argument here is that eviction ranks by read recency, and that only holds once publishing stops overwriting the signal. It also made one sentence of my description false — "the publish path already bumps only changed keys" — now corrected, along with the seeded-contexts caveat you disproved. Regression test On the two consequences you derived: both dissolve once the signal is read-shaped — the horizon evicts again, and cap comparisons stop being ties. I left the comparator without a secondary key deliberately; after the narrowing the only genuine ties are contexts learned from the same remote event, where any order is equally correct. No need for the app-level run. Putting an unofficial build on the machine that reported the bug is a worse trade than the differential replay you already did, which exercises the same write path against real persisted state. One thing this does not fix: the unbounded |
Summary
Marking an older message read is undone by the very write that records it.
Read markers for
msg:/thread:contexts are evicted at two points, and both rank by themarker's value — which is the timestamp of the message that was read, not of the read:
pruneStaleContexts(readStateStorage.ts) drops any prunable marker whose value is olderthan the 7-day horizon, then caps the survivors at
LOCAL_MAX_PRUNABLE_CONTEXTS(1 000),keeping the highest values.
trimContextsToBudget(readStateManager.ts) evicts the lowest values first to fit the32 KB publish budget.
So when a user marks an older message read, the new marker carries that message's old
timestamp, sorts to the bottom of both rankings, and is discarded before it reaches disk or the
relay. The row reads correctly for the rest of the session (in-memory
effectiveStateholds it)and is unread again after the next reload — permanently, no matter how many times it is clicked.
The horizon alone reproduces this at any volume: no message older than 7 days can ever be
durably marked read. The cap makes it bite much sooner on busy accounts.
Evidence from a live account
localStorage["buzz.channel-read-state.v2:<pubkey>"]:Relay side, two
kind:30078slots for the same pubkey:43 780 B of base64 NIP-44 =
1 + 32 + (2 + 32768) + 32bytes, i.e. the padded-plaintext bucketat exactly
READ_STATE_MAX_PLAINTEXT_BYTES. Those 1 024 contexts serialize to roughly 84 KB, sotrimContextsToBudgetis dropping about 60 % of the markers on every publish — oldest messagefirst.
Why the Inbox row cannot recover
resolveInboxItemReadAtresolves a thread row throughmsg:<id>alone, so the channel marker —which is never pruned, and was current the whole time — does not cover it. That is deliberate
(NIP-RS makes the thread fold a MAY, and the per-message predicate is what keeps reading an
ancestor from covering a descendant), so this PR does not touch it. It is what turns the
eviction bug from cosmetic into permanent, which is why it is worth stating.
Fix
Rank eviction by when the read happened, using the signal the manager already persists:
applyRemoteContextTimestamprefreshescontextSourceCreatedAtonly on an actualadvance. Today every republished blob bumps every context it mentions, so the field
collapses to "time of last publish" — on the account above all 1 024 entries land inside a
2-day window, including channel markers first read weeks earlier. Item 4 closes the same
hole on the publish side.
pruneStaleContextsapplies both the horizon and the cap to that recency(
readActionRecency, falling back to the marker value for contexts seeded before thesignal existed).
trimContextsToBudgetevicts least-recently-read first instead of oldest-message first.publishOneSlotno longer stamps recency at all. It used to stamp every key whose valuediffered from
lastPublishedContexts, which is not the narrow filter it looks like:initializeseedslastPublishedContextsfrom the union of the relay blobs while localstorage restores the full context set, so every key
trimContextsToBudgetdropped reads aschanged on the first publish after each launch and is restamped with a single
createdAt.Recency is now written only where a read happens —
markContextRead, and the advance branchof
applyRemoteContextTimestamp, which is also how blob-seeded contexts arrive.Item 4 comes from @rmichelena's independent reproduction below. On his account 330 of 330
prunable contexts carried a recency value but only three distinct ones, all minutes apart —
publish-shaped, not read-shaped. Left in place, that distribution makes the horizon check evict
nothing and turns cap comparisons into ties, so eviction order would fall back to sort stability
rather than read recency. It errs toward keeping markers, so it does not reintroduce the bug,
but the ranking this PR argues for only holds once publishing stops overwriting the signal.
No storage format change, no new key, no wire change. Contexts carrying no recency behave
exactly as before — and in practice there are fewer of those than the first draft of this
description assumed:
mainalready records the signal inmarkContextRead, so existinginstalls get the corrected ranking on upgrade rather than only for reads written afterwards.
trimContextsToBudgetandsplitContextsIntoBudgetedSlotsmove verbatim into a newreadStateBudget.ts. That is not gratuitous:readStateManager.tssat at 999 of the 1 000-lineceiling enforced by
scripts/check-file-sizes.mjs, so the fix could not land in it at all. Bothare pure functions that were already exported solely for unit testing, and the eviction policy
now lives beside the recency helper it ranks by. Manager drops to 846 lines; the new module is
189. No behavioural change in the move itself — happy to reshape the split if you'd rather have
it drawn elsewhere.
Behaviour change worth calling out: the 7-day horizon now measures time since the read
rather than the age of the message read. Same retention window, correct anchor — a marker you
created today survives a week; a marker you have not touched in a week ages out, as intended.
Desktop only.
mobile/lib/shared/read_state/has no equivalent tiered eviction (searched forevict,prune,horizon, byte-budget constants undermobile/lib), so there is no paritychange to make.
Related issue
None found — searched
block/buzzissues and PRs forresolveInboxItemReadAt,useHomeInboxReadState,trimContextsToBudget,read marker eviction,mark as read reverts,inbox unread returns. Closest prior art is #1305 and #1502, which introduced these twoeviction points, and #1242, which routed Inbox rows through per-message markers.
Testing
just cigreen locally (Hermit toolchain, macOS 26.5, Node 24.15):New unit tests, each asserting the old value-ranked behaviour alongside the new one so the
regression is documented rather than merely fixed:
pruneStaleContexts keeps a marker just read on an old messagepruneStaleContexts cap evicts least recently read, not oldest messagewriteStoredReadState keeps a marker just read on an old messagetrimContextsToBudget_evictsLeastRecentlyRead_notOldestMessageapplyRemoteContextTimestamp keeps recency stable across repeated republishespublishing does not refresh read recency— publishes a real blob withlastPublishedContextsempty and asserts an old read keeps its recency; fails on the parent commit, where the recency
jumps to the publish time
No screenshots: this changes storage eviction and read-marker bookkeeping, with no visual,
layout or styling delta. The user-visible effect is an Inbox row staying read across a restart,
which a static capture cannot show.