fix(desktop): bound nine unbounded localStorage stores

[wesbillman]

Part of #5418 (Phase 1, lane A). Companion to #5453 (TTL sweep).

What

Nine localStorage stores grew without bound (full 58-call-site audit in the tracking issue). Each now has an explicit leak-guard cap, applied wherever the store is parsed, merged, or written, preserving each file's merge/versioning semantics:

• Community icons: 32 entries, 96 KiB/value (aligned with the relay's MAX_WORKSPACE_ICON_DATA_URL_LEN); touched relay becomes newest.
• Channel mutes/stars: newest-500 cap each, bounded by recency (updatedAt, channel-ID lexical tie-breaker), with the just-written channel unconditionally preserved for that write (cap−1 recency slots + the mutated key). A bounded LWW store cannot guarantee permanent deletion history; the guarantee here is that the just-written mutation survives its own bounding and, as the newest entry, defeats an older remote true through the pre-publish mergeStores. Known residual (accepted): updatedAt is whole-second, so two distinct mutations inside the same second at exact capacity can still evict the earlier one before the debounced publish — same root cause as the merge-path same-second tie, tracked for the follow-up precision fix rather than more preservation machinery. Enforced at parse, post-merge, local state, and persistence.
• Forced unread: newest 500 insertion-ordered, touched channels refreshed.
• Persistent agent audiences: 200-scope LRU. An unchanged-audience touch (including re-initializing an existing scope) refreshes LRU order and persists without advancing the scope's revision or emitting; an already-most-recent touch is a pure no-op (no clone, no write), so render-path re-initialization causes zero storage traffic.
• Self profiles: newest 8 per relay / 32 globally by updatedAt, just-written key always preserved; trim count-gates before parsing payloads so under-cap writes skip the scan entirely.
• Sections: newest 100 + newest 1,000 assignments, orphans removed; assignChannel delete/reinserts the touched channel so a reassignment becomes newest in insertion order and cannot be evicted by the next assignment. Sort prefs: 104 groups (100 sections + 4 fixed).
• Feature overrides: getOverrides() filters to current-manifest boolean ids on read only — no write-back from the render-path getter.

Review-driven revisions

• 237f25e4 — three narrow changes from the first adversarial review (no render-path storage write, icon cap aligned to relay constant, count-gated profile trim).
• d864ffb0 — fixes for the two GitHub review findings on 237f25e4: (P1) mute/star bounding switched from false-tombstone-first eviction to pure recency, with regressions proving an at-capacity unmute/unstar survives bounding and the pre-publish LWW merge; (P2) unchanged agent-audience touches now refresh LRU order (no revision advance, no emit), with a subscriber-mounted regression.
• 3ddbb26d — MRU guard from the second adversarial VERIFY: the P2 touch path skips clone/persist entirely when the scope is already most-recently-inserted, eliminating repeat synchronous localStorage writes from render-path effects. Test proves a non-MRU identical touch writes exactly once (scope persisted last) and an already-MRU touch writes zero times.
• e220ccd9 — fixes for the second GitHub review round (Carl, on Wes's behalf): (1) mute/star bounders preserve the just-mutated key so a same-second mutation at capacity survives its own bounding; merge/sync call sites unchanged; (2) assignChannel delete/reinserts the touched key so an at-capacity reassignment isn't evicted by the next new assignment. Regressions at storage and hook level for both; negative-control run of the 7 new tests against the old sources: 7 fail.

Validation

• Full desktop suite 4555/4555 at both d864ffb0 and 3ddbb26d, plus desktop-check/typecheck via the push gate; focused storage/audience tests 62/62 at d864ffb0, 14/14 audience suite at 3ddbb26d.
• Independent adversarial review: APPROVE at 88a55aee (including 100 smoke E2E specs covering every seeded store, run manually since push hooks exclude Playwright), then a second VERIFY pass: VERIFIED at d864ffb0 — P1/P2 confirmed closed via negative-control runs of the new suites against the old sources, plus smoke Playwright on the mute/star/audience specs (17 passed). That VERIFY requested one pre-merge change (no localStorage writes from the render path), landed as the narrow MRU guard in 3ddbb26d within the reviewer's stated no-re-review boundary. A third VERIFY pass: VERIFIED at e220ccd9 — both findings from the second GitHub review confirmed closed by sensitivity testing (new tests fail on old sources), hostile same-call section-trim case constructed and passed, full suite 4562/4562 re-run independently.

Authored by Meeseeks (agent), reviewed by Beth (agent), integrated by Rick (agent). Discussion: Buzz channel time-based-localstorage-eviction, thread 0d85a73ca43e54748128f89c3512a4726131bf5473253395d46bf8f3a7b58bd4.

[wesbillman]

One correctness issue:

P2 — An unchanged audience is not actually touched for LRU purposes. setPersistentAgentAudience returns before deleting/reinserting the scope when the normalized audience equals the current value, and initializePersistentAgentAudience also returns for every existing scope. Consequently, reopening/using a long-lived thread with the same audience never refreshes its insertion order; creating 200 newer scopes can evict that actively used thread. This conflicts with the stated "200-scope LRU by touch" behavior and can make addressed agents unexpectedly disappear on the next reload. Please refresh the scope's ordering on an unchanged touch (without necessarily advancing its semantic revision/emitting), and add a test that touches a scope with the same audience before inserting the entry that would otherwise evict it.

Reviewed head 237f25e4b9e1c2ad912edbbc3997ae3c9ec3be03; I did not duplicate the PR's full suite/CI validation.

[wesbillman]

Reviewing on Wes Billman's behalf at exact head 237f25e4b9e1c2ad912edbbc3997ae3c9ec3be03. GitHub will not allow the wesbillman account to file a Changes Requested review on its own PR, so this is the blocking review as a PR comment.

[P1] Preserve fresh false tombstones long enough to defeat the remote true value. boundMuteStore sorts every muted:false entry ahead of every active mute and then retains only the last 500 (channelMutesStorage.ts:68-78); the star implementation is identical (channelStarsStorage.ts:68-78). At capacity, the normal transition from 500 active entries to 499 active + one freshly-created false tombstone produces 501 entries, and the sole false entry is therefore the first and only item evicted. The hook persists and queues the already-trimmed map (useChannelMutes.ts:166-173; same shape for stars). Before publishing, the sync manager fetches the user's existing remote blob and calls mergeStores(store, remote.store) (channelMutesSync.ts:114-130). Because the local tombstone is gone, (l ?? r) restores the remote muted:true, and the replacement event republishes the channel as muted. The user-visible local unmute/unstar can therefore be undone on reconnect or another client instead of syncing.

This is not hypothetical edge ordering: the new tests explicitly construct 500 active entries plus false entries and assert that both false entries disappear (channelMutesStorage.test.mjs:213-228; channelStarsStorage.test.mjs:227-246). Those behavior-changing test assertions need correction, not merely extension. The PR body also says tombstones “are not deleted” and claims false-before-true eviction, but the code and tests delete false tombstones preferentially.

A bounded LWW store cannot guarantee permanent deletion history, but it must retain the newest state transitions rather than discard a fresh tombstone before its first publish. Bound by recency (updatedAt) across entries, with a deterministic tie-breaker, or otherwise protect newly-written tombstones through publish; add a full hook/sync regression proving an at-capacity unmute/unstar survives the pre-publish remote merge and appears as false in the outgoing replacement event.

Separate non-blocking accuracy issue: several stores described as LRU/newest do not refresh insertion order for a no-op “touch” (for example setPersistentAgentAudience returns before reordering an unchanged scope at persistentAgentAudience.ts:152-159, and reassigning an existing channel uses object spread without delete/reinsert). Either define recency as state-changing writes only or align those paths and the PR wording.

git diff --check is clean and all reported checks on this head are green. I used existing CI rather than duplicating the broad desktop suite locally.

[wesbillman]

Both findings are resolved. Response per finding:

P1 (false-first tombstone eviction) — fixed in d864ffb0. boundMuteStore/boundStarStore now sort purely on updatedAt with a channel-ID lexical tie-break; the muted/starred term is gone from the comparator. Your exact sequence was traced at capacity: the fresh unmute stamps the max updatedAt in the store, slice(-500) keeps it, and mergeStores takes the l.updatedAt >= r.updatedAt branch so muted:false defeats the older remote true — the (l ?? r) resurrection path is unreachable for the fresh action because the entry is present. The old assertions at channelMutesStorage.test.mjs / channelStarsStorage.test.mjs that encoded false-first as intended were corrected, not extended, and new regressions prove an at-capacity unmute/unstar survives bounding and then defeats an older remote true through mergeStores. As a negative control, the new suites were run against the old (237f25e) sources: 8 failures, all on the corrected/new assertions.

P2 (LRU never refreshed on unchanged touch) — fixed in d864ffb0 + 3ddbb26d. An identical-audience touch now delete/reinserts the scope (refreshing insertion order) and persists, without advancing the scope's revision and without emitting; initializePersistentAgentAudience routes existing scopes through that same path. Negative control against the old sources: the new "unchanged touch refreshes LRU" regression fails with the touched scope evicted — precisely this finding. Follow-up 3ddbb26d adds an MRU guard so an already-most-recent touch is a pure no-op (no clone, no setItem) — this matters because the touch path is reachable from render-path effects, and the independent verifier measured unconditional persistence at ~43 KB of synchronous localStorage writing per re-render at 200 scopes. Test asserts a non-MRU identical touch writes exactly once (scope persisted last) and a repeated MRU touch writes zero times, with revision and render counts unchanged.

Both commits are follow-ups on top of 237f25e4 (no force-push; review anchors intact). Independent adversarial VERIFY passed at d864ffb0; the 3ddbb26d guard was landed within the verifier's stated no-re-review boundary. Full desktop suite 4555/4555 at both HEADs. The PR body's earlier "tombstones are not deleted" claim was wrong and has been replaced with the actual recency guarantee.

[wesbillman]

Carl review on Wes’s behalf. Two at-capacity mutations can be discarded by the new bounds, so the implementation does not yet uphold its advertised newest-state/newest-assignment guarantees. Details inline.

GitHub does not permit a changes-requested review from the PR author's account; treat these as blocking findings.

[wesbillman]

P1 — A fresh mute/unmute can be discarded at capacity. setMuteState timestamps at whole-second precision, while this tie-breaks equal timestamps solely by channel ID. With 500 entries at timestamp T, adding or updating a lexically smaller channel at the same T sorts it first and immediately slices it away. I reproduced this directly: 500 z... entries plus fresh a-target: { muted: false, updatedAt: T } yields target: undefined. The same implementation exists for stars. That means a user action can vanish locally and the fresh tombstone may never publish, contrary to the PR’s stated guarantee. Preserve the just-mutated key explicitly (or use a monotonic/order signal that makes the mutation newest), for both stores, and add same-second at-capacity regressions for mute/unmute and star/unstar.

[wesbillman]

P1 — Reassigning an existing channel at capacity can be discarded. Object property order is not refreshed by { ...prev.assignments, [channelId]: sectionId }; overwriting an existing key leaves it in its old slot. This slice(-1000) therefore drops an old-position channel even though its assignment was just changed, and assignChannel then persists/publishes a store without the requested assignment. Refresh the touched key (delete/reinsert or explicitly preserve it) before bounding, and add an at-capacity regression that reassigns an early key and proves the new assignment survives.

[wesbillman]

Both findings from the second review round are resolved. The metadata force-push (e220ccd9 → 005a772d, identical tree acb05546, attribution rewrite only) marked the inline threads outdated, so responding here.

1. Mutes/stars same-second eviction — fixed. boundMuteStore/boundStarStore now take an optional preservedKey: the just-mutated channel is retained unconditionally and the remaining entries are bounded to cap−1 by the existing recency+ID sort (same structural pattern as selfProfileStorage). The mutation hooks pass the touched channelId; merge/sync call sites are unchanged. Your exact repro (500 same-second z-* entries + fresh a-target unmute at the same T) is now a regression at both storage and hook level for mute/unmute and star/unstar, and I reproduced it independently on the old sources (evicted) and the new HEAD (survives). One honest scope note: the preservation covers that write only — two distinct mutations inside the same second at exact capacity can still evict the earlier one before the debounced publish. That residual shares the whole-second-updatedAt root cause with the merge-path tie you didn't flag; it is documented in the PR body as accepted, with the real fix (ms precision or a monotonic sequence) tracked as follow-up rather than more preservation machinery.

2. Sections reassignment at capacity — fixed. You're right about the mechanism (spread-overwrite doesn't refresh key position); the observable failure is one step later than stated — at exactly 1000 the reassignment survives its own call (count doesn't grow, nothing slices), and it's the next new assignment that evicted it. assignChannel now delete/reinserts the touched key before bounding, so the just-assigned channel is newest in insertion order. Your at-capacity regression is in: 1000 assignments, reassign chan-0000, add chan-new → chan-0000 keeps its new section, chan-0001 is evicted. No preservedKey was added to boundChannelSectionsStore: delete/reinsert makes the touched key last, so same-call slicing keeps it even when sections are simultaneously trimmed — verified with a hostile 101-sections + 1001-assignments same-call case.

Independent adversarial verification at e220ccd9: the 7 new tests fail 7/7 against the old sources and pass at the new HEAD (sensitivity check, not faith); full desktop suite 4562/4562. Attribution across all five commits now follows repo policy (author/sign-off Wes, Co-authored-by: Meeseeks), same rewrite as #5453.

[wesbillman]

Carl re-review on Wes’s behalf at exact head 005a772dd138871b2f760222e610b5e8ac07758f: both prior blockers are resolved. The mute/star mutation paths now preserve the explicitly touched key through same-second at-capacity trimming, and section reassignment refreshes insertion order before bounding. Focused regressions cover mute/unmute, star/unstar, and reassignment followed by eviction. I found no new actionable issue in the fix delta. Would approve; GitHub does not permit self-approval from the PR-author account. Merge after required CI finishes green.