Repository navigation
fix(profiles): cache profiles on disk and keep dm partners through eviction - #2178
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot review lanes.
Reviewed head 35246e29425f75e6dcef0efe0f6379a3079fca3d against base ffa65d87bb13c85284b3d0fbd6c96a8fc2008ea8. Two P2 findings are detailed inline: ordinary cached-to-live startup can erase the profile mirror before the successor reads it, and profile-only writes bypass the shared startup-store disk budget.
Source review only, including callers, lifecycle, changed test source and public PR description (no attachments). No tests, builds or app workflows were executed; restart timing, browser IndexedDB behavior and the PR’s reported test results were not independently validated.
35246e2 to
a1c31aa
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated follow-up source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Reviewed head a1c31aa847df01ebbc7f75c62b2a748555bc52b2 against base 6dc462a2d6e2b4cc0152752cabcf30bb95822e54, including the endpoint changes since reviewed head 35246e29425f75e6dcef0efe0f6379a3079fca3d.
Both prior P2 findings are addressed in source: restoreStartup() immediately queues the verified mirror rewrite after restoring profiles, before the live successor reads the partition; writeProfiles() now applies the existing global startup-store eviction within the same readwrite transaction. Genuine access-loss purges remain intact. No new actionable findings in the repairs.
Validation limits: source review only; no tests, builds or app workflows executed. The added startup assertion checks the mirror immediately after restore using the in-memory fixture; it does not exercise the complete cached-to-live service handoff, browser IndexedDB ordering or eviction across scopes. The PR’s reported test results were not independently validated. Public PR description inspected; no attachments.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated follow-up source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Reviewed head 2abc214c53c6fbfca83257c936ec6d5dfd46f553 against base 6dc462a2d6e2b4cc0152752cabcf30bb95822e54, focusing on the persistence and regression-test changes since a1c31aa847df01ebbc7f75c62b2a748555bc52b2.
No new actionable findings. The shared startup-store pruning now retains roster/startup rows before optional profile mirrors, preserving the prerequisite for restoring those names while maintaining the 8 MiB budget. Both write paths still prune within their readwrite transaction. The earlier immediate-restore rewrite and genuine access-loss purge repairs remain intact.
Retention tradeoff: startup rows from other scopes, including ones whose discovery has expired, can displace a newly written mirror under budget pressure. Those rows also hold sidebar preferences; treating discovery expiry alone as permission to delete them would require a separate policy decision.
Validation limits: source review only; no tests, builds or app workflows executed. The added mocked request/callback test distinguishes roster-first pruning from the prior newest-first ordering. It does not establish real IndexedDB ordering, eviction across multiple scopes, or complete cached-to-live service handoff behavior. Reported test results were not independently validated. Public PR description inspected; no attachments.
salman1993
left a comment
There was a problem hiding this comment.
🤖 Reviewed 2abc214c5: two P2 findings below. Source review and hosted CI logs only; no local tests or app run.
bb52a4a to
45e4596
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated follow-up source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Reviewed head 45e4596e8f4c3829052111ae17c98ca93077dcac against base ef5a784ec7f0fe492f56a0e441f1510f55ce6acd, focusing on the changed profile restoration and DM-retention behavior since 2abc214c53c6fbfca83257c936ec6d5dfd46f553.
Two P2 findings are detailed inline: parallel roster restoration can abort and overwrite the profile mirror, and protected profiles can exceed the aggregate memory budget and prevent unprotected names from staying cached.
The prior global disk-budget repair remains intact. Retaining profiles across channel access loss matches the updated contract. The explicit background reread and browser row-format changes address the corresponding earlier feedback in source; the new restore-overlap finding concerns the path that exits before that reread.
Source-only review, including callers, lifecycle, changed test source and the public PR description/commit text (no attachments). No tests, builds or app workflows were executed. Startup timing, real IndexedDB behavior, memory behavior and reported test results were not independently exercised.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Reviewed 45e4596e8 from source. I agree with both of the open findings from the last automated review and traced them through the code (details inline). The first one blocks: on an ordinary restart the profile restore usually aborts, and the partial set it holds then overwrites the saved mirror, so names show as keys again on the next launch. The second is less likely but still breaks the profile directory's memory limit.
The two red CI jobs (WebKit notifications.spec.mjs:274 and a Chromium flake-check page.goto timeout in mention-edit.spec.mjs) don't look related to this change.
mirror up to 1024 most recently used kind 0 events into a viewer:relay:profiles row in the startup store. restore re-verifies each event, accepts them alongside the saved roster rather than behind it, and rereads them in the background so a newer kind 0 replaces the cached one and is saved back. saves are debounced to 1s and skip unchanged event ids. a failed disk read never overwrites the saved copy. like the in-memory directory since #2187, only a full cache clear empties the mirror; a denied channel list re-saves it after its clear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
the store protects dm participants in the profile directory whenever the channel list changes, bounded to half the base budget. bytelru eviction skips protected keys, and recent() lists them last so the disk mirror keeps them before anyone else. a large roster can no longer turn a dm label back into a key and re-sort the sidebar. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
the dm label spec polls the profile mirror row instead of the removed discovery.profiles field, and specs that rewrite the saved roster skip the mirror row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ted bytes an access purge during cached startup moved the channel epoch, so the profile restore gave up before accepting the mirror and then saved the partial directory over it. the restore is now fenced only by a full cache clear or disposal, and a disposed restore no longer saves. bytelru evicted every unprotected entry and then stopped while protected entries alone exceeded the byte budget. eviction now takes unprotected entries first and protected ones after, so both limits always hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
45e4596 to
dea070e
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-reviewed at dea070ed0. Both findings from the last review are fixed, and I found no new issues.
- Profile restore aborting on ordinary startup: the restore is now fenced by a counter that only a full cache clear moves, plus disposal, instead of the channel
epochthat an access purge moves. A disposed restore no longer opens the save gate or writes. After a full clear it still saves the in-memory directory, which is the same as the earlier head and the same as any later directory change. - Protected profiles bypassing the byte limit:
ByteLru.evict()evicts unprotected entries first, then protected ones oldest first, until both limits hold. That also coversresize(). No otherByteLruuser passeskeep, so nothing else changes.
I ran the relay startup, store, persistence and profile-details tests at this head: 130 pass. With only store.ts and budget.ts reverted to the previous head, the two new regression tests fail (the names stay missing after the multi-batch restore, and the byte count reaches 2.8 MB against a 2 MiB limit). CI is green.
…able-page * origin/main: feat(browser): show a refused Builderlab app as a page in the panel (#2680) fix(profiles): cache profiles on disk and keep dm partners through eviction (#2178) feat(profiles): let each reader show verified people by username or full name (#2356) fix(media): show transparent avatars without a black background (#2477) fix(channels): keep the reader's place when Back returns to a link-opened channel (#2500) BUZZ-518: Explain required community sign-in (#2710) feat(desktop): serve credentials to agents and your CLI, with pairing (#2260) test(solo): wait for search and access before Share focus reveal (#2573) fix(relay): answer bytes a failed spool wrote; deflake the memory read (#1970) ci(release): publish a signed Intel macOS DMG (#1825) Signed-off-by: Nathan Thillairajah <nthillairajah@squareup.com> # Conflicts: # docs/browser.md # src-tauri/src/browser/builderlab.rs # src/features/browser/BrowserHostView.test.tsx # src/features/browser/BrowserHostView.tsx # src/features/browser/platform.test.ts
This PR combines three changes that cut main-thread work caused by profile updates. #2178 added an IndexedDB mirror of recent profiles: the mirror is no longer rewritten on reads that only reorder it, and restore checks the saved copy with the native batch verifier instead of one event at a time, both on a connected session and in the cached-only restore before the first connection. #2366 caches folded unread rows: a profile notification that changes no agent owner no longer empties that cache. ## Problem **Mirror rewrites.** #2178 mirrors the 1,024 most recently used profiles to IndexedDB. `saveProfiles` skips a write only when the ids, in LRU order, equal the last written list. Every `ensure()` cache hit moves a profile to the recent end of the LRU, so the order changes while no profile does. Each profile directory notification schedules a save one second later. A save sizes the events with `JSON.stringify`, structured-clones up to 1,024 kind 0 events into IndexedDB and reads every row of the store back to prune it. Reads that change only recency therefore rewrite the whole mirror. **Restore.** #2178 restores up to 1,024 saved profiles at session start, so names show at once. `restoreProfiles` checks each saved kind 0 event one at a time with the JavaScript `verifyEvent`, in chunks of 12. At about 1 ms per event, that adds about 0.85–1.4 s of main-thread work at startup. The desktop app already verifies relay reads with the host's native batch signature check, but restore does not use it. **Folded rows.** #2366 keeps the folded rows of each channel in `createUnread`, so a read mark stops refolding every observed event. The same PR's profile subscription calls `foldedRows.clear()` before it checks whether any agent owner changed. Folds read profiles only through `agentOwner`, and the owners check right after the clear already skips the republish when no recorded answer changed. A profile directory notification that changes only a name, an avatar or an attestation therefore still empties the cache. The next inbox or activity projection then refolds every channel, and each fold runs `foldMessages` over all observed events. ## Change **Mirror rewrites.** - `saveProfiles` compares a sorted id signature instead of the LRU-ordered one. **Restore.** - `ReadTransport` gains an optional `verifyStored(values)`. `connectSignedTransport` provides it only when the signer has a native verifier, and it reuses the read path's `parseEvents`, which already does the id, shape and batched native signature checks. - `restoreProfiles` calls it once for the whole saved batch and keeps the kind 0 events. - If `verifyStored` is missing or rejects, restore runs the existing per-record JavaScript loop unchanged. A rejection can mean a bad record, a wrong-length answer or a host failure. - The session passes the transport's `verifyStored` through to the channel store. - The cached-only restore before the first relay connection gets the same check: `provideRelay` and `createCommunities` take a `verifyStored` built on the host's `relay_verify_signatures` command, passed only when the desktop app has an identity, and the relay session hands it to that restore's transport stub. The command needs no connection. `verifyNativeSignatures` is extracted so the signer and this verifier share one command wrapper. **Folded rows.** - `foldedRows.clear()` moves below the owners check. It now runs together with `ownersRead.clear()` and `publish()`, only when an owner answer changed or a fold saw one change. The comment states why folded rows can be kept otherwise. ## No behavior changes **Mirror rewrites.** The mirror is still written whenever the set of ids changes, including when a profile is replaced by a newer kind 0, which has a new event id. A failed write and a cache clear still force the next save to write. In memory nothing changes. On disk, the stored order, which restore hands to the directory as recency, is refreshed with the next change to the set of ids instead of after each reorder. A new test saves two profiles, then runs a cache hit plus an unanswered id, which notifies the directory without a new profile. It expects no write. **Restore.** Every restored event is still id-checked and signature-checked before it is shown. The batch check is fail-closed: one bad record rejects the batch, then the per-record loop drops only the bad records, as before. A host failure takes the same fallback, so it never empties the saved copy. Clear and dispose during the native call still stop the restore. The browser build, and the desktop app without an identity, have no native verifier and run the old loop. The cached-only restore follows the same accept, reject and fallback rules as the connected one, because both go through `parseEvents`. New tests in `startup.test.ts` cover three host answers. Each uses a record whose id is valid but whose signature is not, so only the host's answer decides it: - **Host accepts everything:** the record is restored through the native path with a single host call. The test fails if the wiring is dropped. - **Host rejects that record:** the fallback drops only that record. - **Host throws:** every valid name is restored through the fallback, and the saved copy is kept. New tests in `native.test.ts` and `startup-service.test.ts` cover the cached-only restore: it verifies through the host without connecting and makes no relay query, and it falls back to the JavaScript loop when the host is missing or rejects. Dropping the wiring in `service.ts` fails two of them. **Folded rows.** Folded rows depend on profiles only through `agentOwner`. When every recorded owner answer still matches the directory and no lookup has seen an answer change since (`ownersMixed`), a notification cannot change what a fold returns, so the cached rows are still correct. Every other notification clears the cache and republishes as before, and the existing test for activity folded before an owner answer changed still passes. A new test delivers a display-only notification, then a read mark. It expects one owner lookup, the check itself, instead of one more per refolded channel. It fails on main with three lookups. ## Evidence The table compares main (`8c0cb0c1e`) with this PR as a whole. Every benchmark ran on the same two builds, interleaved on two Linux (32-core) and two macOS (arm64) workstations. Each cell is the mean of the two workstations per OS, and each workstation value is the median of 6 runs. - **Mirror:** 1,024 cached profiles of about 700 B each, over 200 rounds. Each round makes 24 cache-hit `ensure()` calls, which only reorder the LRU, plus one unknown id answered empty, then advances the 1 s save timer. `writeProfiles` is a stand-in that sizes the events with JSON and structured-clones them. It has no real IndexedDB and no prune read, so write time understates the real cost. - **Restore:** restores 1,024 saved kind 0 events at session start. **native** has a stand-in batch verifier that resolves from a macrotask, because libsecp256k1 runs off the main thread in the app. **js** has no native verifier, which is the browser build's path. **Longest task** is the largest gap of a `setImmediate` ticker during restore. - **Folded rows:** 400 channels and 3,000 observed events, one fifth of them from an agent that has an owner, over 30 rounds. Each round delivers a profile notification that changes no owner, then a read mark, then an inbox read. - **Browser:** production Chromium at 4× CPU throttling with 128 DMs, 1,001 participants, read state and a 5,080-member channel, while another channel receives a message every 200 ms. **Idle** is 3 s with no input while the 5k channel is open. **Switch** opens Cmd+K, types the channel name and presses Enter. 36 samples per cell per OS. | metric | Linux: main | Linux: this PR | Linux Δ % | macOS: main | macOS: this PR | macOS Δ % | |---|---|---|---|---|---|---| | mirror, IndexedDB writes | 200 | 0 | −100% | 200 | 0 | −100% | | mirror, write time | 537.8 ms | 0 ms | −100% | 295.2 ms | 0 ms | −100% | | mirror, total for 200 rounds | 705.0 ms | 152.9 ms | −78.3% | 390.7 ms | 84.2 ms | −78.4% | | restore native, total | 1,381.3 ms | 16.0 ms | −98.8% | 849.2 ms | 10.4 ms | −98.8% | | restore native, longest task | 20.7 ms | 3.0 ms | −85.7% | 12.1 ms | 1.4 ms | −88.8% | | restore js, total (control) | 1,385.5 ms | 1,396.1 ms | +0.8% (noise) | 856.2 ms | 868.0 ms | +1.4% (noise) | | folded rows, `foldMessages` calls | 12,000 | 0 | −100% | 12,000 | 0 | −100% | | folded rows, time for 30 rounds | 7,917.7 ms | 1,368.9 ms | −82.7% | 2,980.8 ms | 676.3 ms | −77.3% | | browser idle, script | 252.3 ms | 259.7 ms | +3.0% (noise) | 147.0 ms | 148.2 ms | +0.8% (noise) | | browser switch, script | 609.1 ms | 623.1 ms | +2.3% (noise) | 321.6 ms | 323.9 ms | +0.7% (noise) | | browser switch, blocking | 324.3 ms | 322.5 ms | −0.5% (noise) | 114.6 ms | 116.2 ms | +1.5% (noise) | The cached-only restore change is not in the table, because it needs the desktop host. A separate browser model of that restore, with 1,024 signed profiles in real IndexedDB, cuts the time to the first cached profile publication from 435.7 ms to 85.3 ms (−80.4%) against this PR without it, median of 6 interleaved starts per arm on Linux. The host call there is a timer set to the measured Rust batch time (32.1 ms median for 1,024 signatures) plus an assumed 5 ms of IPC, so this is a model, not a measured app startup. With no saved profiles the two arms match (−0.6%). Lower is better. Blocking is long-animation-frame blocking time. The stand-in verifier does no curve math, so the native restore rows show the main-thread work removed rather than end-to-end restore time in the app. The browser harness uses the JavaScript path, so restore cannot be measured there. The Linux browser script rows differ between the two workstations (idle +4.3% and +1.2%, switch +5.0% and −0.6%), and macOS stays within 1.5%, so they are within run-to-run noise. Those scenarios do not hit the reorder-only reads or owner-free profile notifications that the micro-benchmarks isolate. Checks: the relay and communities unit suites pass (3,537 tests), along with `tsc` and `biome`. The new folded-rows test fails on main. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Kalvin Chau <kalvin@block.xyz> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Problem
After an app restart, or a
service.retry()out of an error state, names and avatars show as public keys. The new session's profile directory starts empty. Only two kinds of names come back from disk: authors of saved channel heads, and DM participants (discovery.profiles). Members, mentions, owners, inbox and huddle names stay as keys until a view asks the relay again.Change
persistence.ts: addsreadProfilesandwriteProfiles.${viewer}:${relay}:profilesrow in the existingstartupobject store. No IndexedDB version bump.clear()deletes the row.profile-directory.ts:protect(ids), capped at 512.ByteLrugains an optionalkeeppredicate. Eviction removes unprotected entries first and protected ones only after that, so both the entry and byte limits always hold.recent(count), which returns held kind 0 events least valued first: protected identities last, everyone else by least recent use.store.ts:PROFILE_CACHE_BASE) after a quiet second. The write is skipped when the event ids are unchanged.eventDto) and accepted into the directory in chunks that yield to the host. This runs next to the saved roster, not behind it: since fix(relay): keep profiles when channel access ends #2187, profiles don't depend on channel access. Only a full cache clear or disposal aborts it; an access purge does not.created_atwins (NIP-01 replaceable rule), and that directory change saves the newer event back. This covers a restore that finishes after the first connection's full reread (profiles.reconnect()).discovery.profilesstartup field. Records saved by older builds load without names once.Invalidation
Residual risk
Tests
startup.test.ts:startup.test.ts: a 60-profile (multi-batch) mirror restores in full when the shorter roster restores first. This fails if an access purge aborts the restore.store.test.ts: one mirror write per burst, and none when the cache is cleared mid-burst.persistence.test.ts: a mirror write over the budget evicts the mirror, not the saved roster.startup.spec.mjs(browser): the DM label spec polls the mirror row instead of the removeddiscovery.profiles, and specs that rewrite the saved roster skip the mirror row. All 25 pass locally on Chromium.profile-details.test.ts: a protected DM partner survives eviction from a full directory and is listed last byrecent(). This fails without the eviction skip. Protected profiles larger than the byte budget still keepstats().byteswithin the limit.tsc(clean), biome on the changed files (clean), and vitest on startup, store, persistence, revocation, prepared, startup-service, channel-references, store-system-discovery and ChannelMembersDialog, plus profile-details, verified-profile, event-proof and outbox. All pass, except two ChannelMembersDialog cases that failed only under the combined parallel run; that file passes alone (105/105).🤖 Generated with Claude Code