perf(desktop): move five hot renderer paths from JS into Rust - #6024
Conversation
Every inbound relay frame was its own `Channel::send`, and every send wakes the main run loop. Under a catch-up storm — reconnect, channel switch, history backfill — that is one wakeup per frame. `run_connection` now coalesces inbound frames into a single delivery over an 8ms window, so N wakeups collapse into 1. The bound is bytes, not frames. `tauri::ipc::Channel::send` forks on payload size: below `MAX_JSON_DIRECT_EXECUTE_THRESHOLD` (8192) it goes straight to `webview.eval`; at or above it the body is parked in a `ChannelDataIpcQueue` and the webview calls *back* into Rust over IPC to fetch it (tauri-2.11.5 `src/ipc/channel.rs:37,154-181,319-331`). That round trip is exactly what batching exists to remove, so a batch must never cross the line. Measured on real relay traffic (n=93) frames run p50 1319B / p90 3913B / max 6012B, so a frame-count bound of 64 would have put every batch on the slow path while looking like a win. Frames are serialized once on arrival and the batch tracks its true serialized length, because JSON escaping inflates payloads by an amount no fixed per-frame estimate can bound (measured 1.06x p50 on real frames, but 5.9x worst-case synthetic for control characters). A frame that exceeds the bound alone is delivered alone, taking the fetch path exactly as it does today. Ordering is FIFO in all cases: buffered frames flush together with the frame that forced the flush, never after it. Only the NIP-42 AUTH challenge bypasses the window — it gates a round trip the relay is waiting on, while OK/EOSE ride the timer so catch-up batching survives. A missed AUTH match costs at most one window against a 25s auth timeout, never correctness. Every delivery is now an array, including the single-frame case. The unwrap lives once in `relayClientShared.toRelayFrames` rather than per client: `getTextPayload` returns null for arrays, so an un-unwrapped consumer would drop batched frames silently instead of failing. All three consumers go through it, including the e2e bridge — which now emits batched shapes, so the e2e suites cannot green against a transport shape production no longer sends. Verification: - 19 new Rust tests. A 6-mutant battery (oversized bound, AUTH never urgent, no final flush, no straddle flush, FIFO reversed, timer never fires) kills all 6, each by its own distinct guard. The first three mutants initially *survived* because those tests drove `FrameBatch` directly and bypassed the loop's real flush policy; they now run against `run_connection` itself over an in-memory duplex socket with a paused clock. - `cargo test --workspace` 2452 lib + 27 terminal + integration, 0 failed. `cargo clippy --workspace --all-targets -D warnings` clean. `cargo fmt --check` clean. - 4958 TS tests pass, including 4 new `toRelayFrames` tests; removing the unwrap kills 3 of them. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
The renderer owned the whole archive path: it listed saved subscriptions, opened a live REQ per scope, buffered inbound frames, batched them, and handed each batch back over IPC to be written to SQLite. Every archived frame therefore made a round trip into JS for no reason other than history — nothing in that path is a rendering concern. `archive::sync` now runs the whole pipeline natively. It subscribes from the saved subscriptions, buffers, flushes on the same thresholds the JS manager used (FLUSH_BATCH_SIZE 25 / 2000ms deadline — parity, not a retune), archives via SQLite directly, and emits `archive-agent-metrics-changed` when a batch actually persisted metric rows. `archiveSyncManager.ts` is deleted. `native_relay_client` is the shared piece underneath: one authenticated socket per (relay, pubkey), multiplexed subscriptions, declarative `set_subscriptions` reconciliation, and exponential reconnect backoff. A 30s read timeout is idle, not failure, and is discriminated by error variant rather than message text so a reworded error cannot turn every quiet period into a reconnect storm. A relay CLOSED drives its own recovery: per-id retry state lives next to `open` in the connection (not in `desired`, which is reloaded from SQLite and would resurrect a deletion), a dedicated select! deadline arm fires the reopen and is disabled when nothing is scheduled, and the CLOSED message is classified terminal / rate-limited / retryable with the same prefixes the renderer used, `auth-required:` deliberately retryable. Rate-limited arms the shared relay_admission gate and waits max(backoff, hint); backoff is 1s→30s saturating; attempts reset on EVENT or EOSE. Terminal suppression is per-socket by design: a reconnect retries a terminal id once through the normal path, because relay policy can change and one REQ per reconnect is bounded. The sync lifecycle is owned, not raced. The renderer allocates a monotonic lease synchronously in effect order — intent order, which IPC completion order is not — and Rust ignores any start/stop older than the highest (epoch, lease) mark it has seen; a stop advances the mark, so a delayed start cannot resurrect a stopped task. Above the lease, Rust mints a realm epoch, published atomically with minting under the same lock that orders lifecycle calls: announcing IS what supersedes the previous realm, so a separately-held counter would leave a window in which a dead realm's delayed calls still win. The renderer awaits the epoch before its first lifecycle command. Ownership is main-window-only via the established huddleWindowChannelId() exclusion: a huddle companion mounts the same tree in a concurrent realm, and concurrent owners cannot be ordered by any newest-wins clock. What stays in JS is the start gate, deliberately. Kind 24200 is relay-ephemeral, so frames emitted before the listener opens are lost permanently, and only the renderer knows when observer reconciliation finished seeding 24200 into the saved subscription. The backend task is therefore not self-starting: `useArchiveSync` starts it once reconciliation resolves and stops it on unmount. `RelaySession.revision` was documented as rejecting stale in-flight reconciliation but never did — removed rather than repaired: declarative reconciliation re-reads the desired set every pass, so for the open set there is no generation to guard. That argument does not extend to CLOSED retry state, whose validity depends on the id having been continuously desired — history that coalesced wakes erase. So departures are recorded at write time: set_subscriptions diffs old against new desired under the one SessionState lock and reconcile snapshots the desired set and drains those departures in a single acquisition, pruning the retry entries they invalidate. Without this, deleting and recreating the same saved subscription (byte-identical id by construction) inherited the old terminal latch and was suppressed for the life of the socket. Two stale-frame races are closed alongside: a CLOSED for an id not in `open` is stale and mints nothing (our own CLOSE raced it, same defense the EVENT arm already had), and an EOSE for an id not in `open` wakes a reconcile — EOSE is the only ordered fence on the wire, and without that wake a stale terminal CLOSED against a recreated id blackholes a live subscription with no timer or wake left to recover it. A subscription id's filter is immutable for the life of a session (a CLOSED carries only the id, so a rejection of the old filter is indistinguishable from one of the new); the write-time diff detects violations and logs them, with post-violation behavior deliberately unspecified. The retries doc carries the full eviction table, including the deliberately omitted absent-from-snapshot prune and the inductive argument for why it is unreachable. Tests: archive/sync_tests.rs drives the real run_sync body through a fake IO seam (filter parity, flush thresholds, failure isolation) plus the ownership contract (out-of-order start/stop both directions, announcement supersedes a dead realm's delayed calls before any new lifecycle call, publish-(epoch,0) does not lock out the announcing realm). native_relay_client's stub-relay test completes the NIP-42 handshake over a real TCP socket, injects CLOSED with the desired set unchanged, and proves the REQ is retried by the deadline arm; its lifecycle suite (split into native_relay_client_tests.rs to stay under the file-size ratchet) pins the retry-eviction contract with six mutation-controlled tests, including the coalesced delete-recreate that discriminates write-time recording from any observe-time prune, and the stale-CLOSED/EOSE-heal schedules — the relay-backed #[ignore] test additionally proves the REQ shape against a real relay. useArchiveSync.test.mjs owns the start gate, realm ownership, and post-reload realm supersession via fresh module instances. observer-archive-policy.spec.ts owns wiring, with payload receipts that announce precedes start and start carries a numeric epoch. Every claim was mutation-checked; the vacuous first drafts (wake-masked reopen, policy re-implementation, precondition-rebuilding supersession, same-scope no-op escape) were each caught by their mutants and rewritten. Includes one move-only hunk that is not archive work: the push-to-talk global-shortcut handler moves from lib.rs into `ptt_shortcut::install`, mirroring the existing `app_menu::install` seam, paying the 1000-line ratchet budget in the module that owns the registration lifecycle. The handler body is proven token-identical with a mutated-body negative control. lib.rs is 917 lines. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
Fetch persona catalog pages through the shared native relay session, verify relay events outside the renderer, preserve NIP-33 head and parser trust semantics (claim precedes shared gate precedes parse), and return one projected DTO across IPC. Keep only catalog-to-local-persona linkage in TypeScript. Signature verification measured at ~473us/event; a full 500-event page cost ~0.24s on the webview thread before this change, so verification runs under spawn_blocking. Finite catalog requests use fresh subscription ids alongside archive subscriptions on the same authenticated socket, fulfilled through the request map. Persistent archive events route directly through one bounded mpsc channel and the send is awaited in the socket loop: archive subscriptions are live-only (limit 0), so replay cannot repair eviction, and throttling a slow consumer preserves the no-loss contract. A slow archive consumer may therefore throttle a concurrent catalog fetch on the shared socket; that is deliberate and preferable to an unrecoverable archive gap. The awaited send sits outside the session-cancel select; teardown relies on run_sync dropping its receiver on its own cancel path. The catalog intentionally verifies finite events twice: transport verification bounds memory against forged input, while catalog verification keeps the projection helper sound for every caller. The 2x ~473us/event cost is deliberate. Real-WebSocket coverage proves request EVENT/EOSE/CLOSE flow, forged-event rejection, continued persistent delivery, and backpressure without loss under a slow-consumer burst (fast and slow arms, 1200/1200 each, plus post-burst delivery). Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
Problem: on wake or channel-list refresh the renderer issued one relay round-trip per not-yet-caught-up channel (N REQs, N awaits, ~175 lines of per-channel JS), and the two-pass notify classification ran per channel as promises resolved, so pass-one roots discovered in one channel were visible to another channel's pass two only if that channel's fetch happened to resolve later. Design: a single `unread_catch_up` Tauri command on the shared native relay session fetches all channels concurrently (Semaphore(8) finite REQs via fetch_events, JoinSet) and classifies globally in two strict passes — gather ALL history first, then classify — so cross-channel pass-one visibility no longer depends on resolution order. The Promise.all completion-order race is unrepresentable, not handled. Per-channel Success/Error results keep retry semantics: the error arm releases the channel's caught-up claim (its only identity) so the next effect run retries it. DiscoveredRoots deltas flow back to the renderer, which still owns membership stores and scope fencing — the command re-checks pubkey+relay after fetch; the renderer keeps isScopeLoaded/isCancelled for effect cleanup the command can't see. Both fences are load-bearing. ACTIVITY_LIMIT 100 is applied as a global pre-cap (newest-100-of-union ⊆ newest-100-of-batch, proven). Wire contract: `ChannelResult` is an internally tagged enum, and serde's `rename_all` on such an enum renames VARIANTS, not variant fields — variant fields need `rename_all_fields`. The initial cut emitted snake_case fields against the camelCase TS contract, breaking every catch-up while all gates ran green (the e2e bridge hand-wrote the intended shape; no test compared emitted bytes to declared types). Both IPC DTO surfaces in the pack are now pinned by whole-value wire tests asserting serde OUTPUT against the TS contract: catch-up (red-run proven at the defective bytes; variant-rename, nested-struct, and tag-key mutants all killed) and the persona catalog (rename_all mutants on both DTOs killed). The e2e bridge result is typed `UnreadCatchUpChannelResult[]`, so a drifting mock fails typecheck instead of silently certifying a stale contract. Interim cost, named where it is paid: membership stays renderer-owned until the native observed-unread store lands, so five root-id sets cross IPC on every catch-up — bounded at 5x1000 ids ~= 332 KiB worst case, linear fetch-body cost with no size cliff. Retiring this is the follow-up store's job, not an interim optimization. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
Persist observed unread events, read markers, membership, and channel projections in a scoped WAL database. Keep renderer mutations ordered with a sequence/revision protocol, migrate localStorage transactionally, and let native catch-up load membership without serializing five capped arrays. Measured at the same populated 5x1000 membership fixture: - before: 335,280 bytes (327.4 KiB) - after: 178 bytes (0.2 KiB) Ack/restart failure matrix: | Failure boundary | Contract | |---|---| | Rust commits sequence N, renderer dies before observing ack | **Handled by replay:** DB ack is durable; reopen returns `lastAckedSequence=N`. Renderer may resend N from its pending local queue; Rust recognizes `N <= ack`, performs no mutation, and returns current revision/snapshot. Event-id idempotence is the second fence. | | Renderer observes ack N, dies before deleting its pending batch | **Handled identically:** replay N is a no-op; no duplicate row or revision bump. | | Renderer deletes pending N without durable ack | **Unrepresentable by construction:** deletion happens only in the resolved-success branch after validating matching scope, sequence, and revision. Reject/throw leaves N queued. | | Scope switch lands while A ingest is running | **Handled:** A request carries immutable scope; transaction commits only to A. Switch drains A first when available, opens B independently, and response merge requires exact current scope; a late A ack cannot advance B's sequence or projection. If drain fails, A's retained unacked batch stays replayable when A reopens. | | Scope switch after A commit but before A ack observation | **Handled by durable ack + scope fence:** A reopen learns ack N; B never sees it. | | Batch N+1 arrives before N / IPC retry reorders | **Explicitly rejected:** only `sequence == ack+1` mutates. `> ack+1` returns `snapshotRequired`/expected sequence; `<= ack` is replay/no-op. Single JS queue makes normal reordering unrepresentable, backend check covers abnormal callers/restarts. | | DB commit succeeds but response serialization/delivery fails | **Handled as lost ack:** resend; durable sequence and idempotent event ids collapse it. | | DB transaction fails halfway (events/markers/prune/revision/ack) | **Unrepresentable:** one SQLite transaction; rollback leaves ack+revision unchanged, so retry is the same next sequence. | | Migration imports rows but renderer dies before seeing marker | **Handled:** rows + migration marker commit atomically. Reopen reports complete and current snapshot; legacy key remains until renderer observes that, then is deleted. Re-sending payload after complete is ignored. | | localStorage delete succeeds, native DB later becomes unavailable | **Handled by one-release fallback limitation explicitly:** fallback can preserve new session events but cannot reconstruct migrated history after confirmed native ownership. Native open failure surfaces and does not mutate/delete legacy data pre-confirmation; DB corruption after confirmed migration is logged/recoverable as degraded state, not silently represented as “zero unread.” I will test this distinction rather than claim impossible loss recovery. | | Revision response gap/out-of-order | **Handled:** apply requires exact `baseRevision`; otherwise discard payload and request full snapshot. Snapshot replacement requires matching scope and `revision >= current`. | | App shutdown during coalesce | **Handled twice:** `pagehide` drains renderer queue; native shutdown flushes SQLite/checkpoint. Already-committed batches need no renderer ack to survive restart. Unsent events inside a renderer killed without pagehide are the irreducible initial-source limit; live relay catch-up can rediscover them, and the existing TS fallback gate remains for native write failures—not arbitrary process kill. | Build extension: each scope carries a generated UUID epoch. A different epoch means the store was rebuilt, so the renderer accepts the replacement snapshot regardless of its old revision and resets sequence/revision. This makes DB recreation distinguishable from a stale snapshot and prevents a revision wedge. Native projection deltas now contain changed channels only. Top-level unread continues to mean an unread observed event whose `rootId` is null, matching the retired renderer helper exactly. Read-state version changes, including relay sync advances, feed marker updates into the same ordered mutation chain. Review round (native-mode coverage and repair): an in-memory __TAURI_INTERNALS__ protocol rig proves native mode is entered rather than silently falling back; local read markers reach the native store with their exact timestamps (not only relay-synced advances); catch-up maxTrigger persists as a monotonic per-channel latest anchor independent of notify-filtered rows; renderer membership seeding is one-shot so later opens cannot erase natively discovered membership. Guard tests call the production helpers (seed_membership_once, advance_channel_latest) and die when the guards die; a badge-lane test asserts the hook's returned unreadChannelCounts/unreadChannelIds under native mode, with mutation controls red-verified for both. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
Teach the E2E Tauri mock the pack's new renderer<->backend contracts and fix two production seams the repaired suites then exposed: - Stateful observed-unread mock: scope open/ingest now derive real per-channel projections (count, badgeCount, appBadgeCount, topLevelUnread, highPriorityUnread) with monotonic channel_latest anchors, mirroring observed_unread.rs instead of returning empty rows. - Persona catalog mock: add the fetch_persona_catalog case, with validation mirroring the Rust validator's emoji semantics (FE0F/ZWJ). - unread_catch_up mock mirrors Rust authored-root discovery: self-authored top-level events are returned as discovered.authored so thread-activity membership matches the native contract; replies stay excluded. - Production: suppress the onPruned notification for true empty projection deltas, breaking a marker-ingest -> no-op delta -> notify -> re-render -> re-ingest feedback loop that saturated the main thread (ingest sequence doubled to 8192 within ~5s). Snapshot and snapshotRequired paths still notify unconditionally; regression tests cover both the no-op suppression and snapshot recovery. - Production: the desktop app-dot fallback now reads topLevelUnreadChannelIds, so thread-preview-only unread no longer lights the app badge while the sidebar thread indicators still do. Marker ingests for genuine projection changes remain fire-per-effect-run; this is bounded now that the no-op cycle is broken. Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
a19d6e5 to
625ee7b
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to 625ee7b72a32397bbb4f3920a86f2f0111580457. I found two correctness blockers and one material performance-boundary issue:
-
Archive sync mixes scopes during identity/community changes.
start_archive_synccaptures the relay session for scope A (desktop/src-tauri/src/archive/sync.rs:503-525), butAppIo::list_subscriptionsandAppIo::archivelater re-read mutable globalAppState(sync.rs:278-300). Cancellation deliberately flushes buffered A events (sync.rs:203-233), yet that flush callsarchive_candidates, which derives identity, relay, relay-query credentials, and commit keys from whichever scope is current then (archive/mod.rs:162-200). If the app switches A→B before the flush, A's ephemeral 24200 events are validated under B and dropped permanently; persistent candidates are likewise planned/queried/committed against the wrong scope. Bind the task's complete archive IO pipeline to the identity, relay, and signer captured at start, and add a production-wiring regression for buffer-A → switch-B → cancel/flush into A only. -
One rejected observed-unread ingest permanently poisons later mutations. Marker, clear, latest-anchor, and membership writes append with
chainRef.current = chainRef.current.then(...)and no rejection handler (desktop/src/features/channels/useObservedUnreadPersistence.ts:336-355,401-420,437-456,466-485). Once oneingestObservedUnreadrejects, every later.thenis skipped.nativeRefremains set andnativeFailedRefremains false, so the hook still reports native persistence as healthy (:525-527). Destructive clears also remove projections optimistically (:491-511), making the UI show cleared state that SQLite can resurrect after restart. Centralize queued mutations behind failure handling that restores the chain and explicitly degrades/requeues or rolls back unacknowledged state. Add a test that rejects ingest and proves subsequent mutations either run or safely enter fallback without losing state. -
The new observed-unread persistence path performs blocking SQLite work directly in synchronous Tauri commands. Both commands take a global
std::sync::Mutex, open/configure SQLite, and project as many as 5,000 rows inline (desktop/src-tauri/src/observed_unread.rs:407-570; projection occurs before and after each ingest at:501and:548). This can block a command/runtime thread, including mutex waits behind the configured SQLite busy timeout. The archive path correctly uses the blocking pool for the same class of work. Move the complete lock/open/transaction/projection unit tospawn_blockingor a dedicated DB worker, then benchmark a populated store and verify unrelated command responsiveness. This matters especially because this PR is sold as a performance improvement and its paired CPU benchmark remains unrun.
CI is green, and git diff --check is clean at the pinned head. Those gates do not exercise either scope switching or ingest rejection, and the PR itself records that live native observed-unread persistence and paired CPU improvement were not demonstrated. Please fix the two data-integrity/recovery defects before merge and resolve the blocking-runtime risk with implementation or evidence.
…the mutation chain Two defects Carl's review found in the native observed-unread read model, both on paths no gate exercised. The commands were sync `#[tauri::command]`, which Tauri treats as `ExecutionContext::Blocking` and runs inline in the IPC handler — the main thread on macOS. Each call projects the whole scope twice, so at the pack benchmark's own fixture (15 channels / 5000 events) one ingest held the main thread 7.2 ms release / 27.6 ms debug, against a 16.7 ms frame budget; catch-up issues them per discovered root, so a 50-root burst measured ~340 ms release. Both now run on the blocking pool, matching `archive_events` in this same crate. `blocking::run` mints an `OnBlockingThread` token that the bodies require and nothing outside the module can construct, so neither regression compiles: dropping `async` leaves nothing to await, and calling a body directly leaves no way to obtain the token. A rejected ingest also left `chainRef` a rejected promise. Every later `.then` was skipped while `isNative()` still reported the native path healthy, so marker, clear, and membership writes vanished silently — and `removeChannel` /`clearAll` delete their projections optimistically first, so the UI showed cleared state the store still held. All four direct mutators now route through one `enqueueNative` helper whose rejection path reopens the authoritative snapshot; if that reopen also fails, native mode is declared unhealthy rather than lying. Reopen results are fenced to the still-loaded scope. The third review item — archive flush re-deriving identity from current `AppState` after an identity switch — is real but predates this pack (base did the same through an unawaited `archiveEvents`) and lives in another subsystem. Filed separately rather than widened into this PR. Co-authored-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to f079d0914adc4ae8ac5ff1bac13e9a8dfb806548. The latest commit correctly moves observed-unread SQLite work off the IPC/main thread and prevents one rejected promise from poisoning every later mutation, but two data-integrity/lifecycle blockers remain:
-
Recovery silently drops the mutation whose ingest failed.
enqueueNativecatches rejection by reopening the authoritative snapshot, then resolves without retrying, requeuing, or otherwise preserving its capturedmutation(desktop/src/features/channels/useObservedUnreadPersistence.ts:256-289). Every direct non-event mutation uses this helper: markers (:370-396), destructive clears (:434-449), latest anchors (:452-470), and membership (:473-487). In particular,removeChannel/clearAllfirst delete the projection optimistically (:489-511); a rejection followed by reopen replaces that projection from stale SQLite (:109-135) while discarding the user's clear. Marker updates are also sent once frommarkChannelRead(desktop/src/features/channels/useUnreadChannels.ts:345-360), and the native read-state effect does not replay them (useObservedUnreadPersistence.ts:398-417). The added recovery test demonstrates the hole rather than closing it: it failschannel-failed, removes the failure, and asserts only that a laterchannel-nextlands (observedUnreadNative.test.mjs:188-240). Preserve the original operation, for example by retrying it once against the reopened sequence/revision and explicitly degrading with equivalent fallback state if recovery still fails. Add regressions for the originally rejected marker, destructive clear, and membership delta, not merely the next command. -
A superseded archive start can tear down the newer scope’s shared relay session.
ArchiveSyncState::begininstalls ownership and cancels the prior task (desktop/src-tauri/src/archive/sync.rs:413-443), butstart_archive_syncthen awaitsarchive_sessionafter releasing that ownership guard and never revalidates its mark/cancellation before spawning (sync.rs:511-529).NativeRelayClient::ensure_sessionindependently makes its latest caller authoritative and shuts down a different current scope (desktop/src-tauri/src/native_relay_client.rs:94-108). Thus old start B can winbeginand pause; newer C can winbegin, cancel B, and install session C; then stale B resumes, shuts down C while installing session B, and immediately runs with an already-cancelled archive token. B clears its subscriptions on exit while C owns a shut-down session, leaving archive sync dead until another lifecycle edge. Fence session acquisition/installation with the same ownership mark, and add a production-wiring regression that pauses B afterbegin, fully installs C, resumes B, and proves only C’s session and subscriptions remain live.
All exact-head GitHub checks are green and git diff --check is clean. I did not duplicate those suites locally. The current tests do not exercise either failed-operation durability or the post-begin async installation race. Please fix both before merge.
…ion acquisition Round-2 review found two defects in the previous repair commit. **The mutation whose ingest failed was dropped.** `enqueueNative`'s recovery reopened the authoritative snapshot and resolved, so the captured mutation was never retried or preserved — and the reopened snapshot is the store *without* that write. A rejected marker un-read the channel; a rejected `removeChannel` /`clearAll` resurrected rows the user had just cleared, because those delete their projection optimistically first. The previous test proved the chain healed, not that the operation survived: it asserted only that the *next* command landed. Every captured mutation now gets one retry after `reopen` refreshes sequence and revision, which is safe because ingest is idempotent — events upsert `DO NOTHING`, channel latest advances by `MAX`, membership is `INSERT OR IGNORE`, and a replayed sequence returns a snapshot rather than reapplying. A `snapshotRequired` response takes the same recovery path instead of being treated as success. If reopen and retry both fail, native is explicitly degraded and the equivalent event/marker/latest/clear state is applied to the JS fallback, so `isNative()` stops reporting healthy. Membership needs no fallback copy: its renderer-owned sets remain authoritative. **A superseded archive start could tear down the newer scope's relay session.** `begin` claimed ownership and released its guards, then `start_archive_sync` awaited `archive_session`. `ensure_session` shuts down a different scope's socket and installs its own inside its own lock, and `attach_archive` replaces the session's archive sender outright — both destructive on entry. So a stale start B could win `begin`, pause, let newer C claim and install, then resume, shut C's session down, and attach the archive stream to a task whose token was already cancelled. B cleared its subscriptions on exit and archive sync stayed dead until the next lifecycle edge. Revalidating the mark after the await cannot fix this: by the time B discovers it lost, C's socket is already gone, and a session is spawned rather than handed back, so there is nothing to restore it from. The damage is done by the call, so the fence is around the call. `begin` now returns an `ArchiveOwnership` token holding both guards, and `archive_session` requires one. The token is un-constructible outside `archive::sync`, so a stale start cannot reach the call at all. This is sound because acquisition performs no I/O: both halves await only mutex acquisitions and the socket connects on a spawned task. Reverting the token does not compile, so the Rust regression pins the property its usefulness rests on — while an owner holds the token, no newer start can claim. That goes red against the mutant this design exists to stop: keeping the token but releasing the guards inside `begin`, which compiles and restores the race. The JS regressions assert the originally rejected marker, destructive clear, and membership delta each survive, and go red when the retry is removed. Co-authored-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
The pack branch was behind `origin/main`, and main had changed two files the pack also touches (`desktop/src-tauri/src/lib.rs`, `desktop/src/testing/e2eBridge.ts`). The pre-push branch-skew guard blocks exactly this: local gates run on a tree CI will never build, so a green local run can coexist with a red CI. Textually clean, no conflicts. Because a clean merge is precisely where two edits to one file can silently recombine, both directions were checked by content rather than by conflict count: all 342 lines the pack added to those two files are present in the merged tree, and all 17 lines main added are present too. Nothing was reverted on either side. Gated on the merged tree, which is what CI actually builds: `cargo test --lib` 2544 passed / 0 failed, `pnpm test` 4956 passed / 0 failed. Co-authored-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
Co-authored-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Signed-off-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> * origin/main: fix(desktop): bind presence retry timers (#6213) ci: make file-size policy a first-class gate (#6187) fix(desktop): eliminate mounted-view CPU burn — compositor-safe shimmer, observer append fast path, poll-tick disk reads (#6198) chore(release): release Buzz Desktop version 0.5.16 (#6191) fix(desktop): restore release agent mentions (#6182) test(desktop): cover exact workflow batch limit (#6168) chore(release): release Buzz Desktop version 0.5.15 (#6173) Preserve managed agent mentions during relay errors (#6167) fix(workflows): preserve multi-channel listing semantics (#6009) Remove Startup Recovery section in base prompt (#6161) fix(desktop): align preview sidebar row styling (#6163) fix(desktop): repair dropped team membership links at boot and on edit (#5904) Signed-off-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Princess Donut, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to 8f865889a5fd4f9b9b346d760b0f59612963a435. Two cross-scope lifecycle regressions remain:
-
BLOCKER — a failed scope-A event flush can inject A’s unread event into scope B and disable B’s native store.
flushNativecaptures A’s queued events, but its rejection handler has no scope fence: it writes those captured events into the hook’s current renderer refs, sets the currentnativeRefto null, and persists under the currentscopeLoadedRef(desktop/src/features/channels/useObservedUnreadPersistence.ts:193-253). A community/identity switch deliberately callsflushNative()and then immediately resets/reopens the mutable refs for B (:355-420). If A’s in-flight ingest rejects after B opens, the catch at:230-253records A’s event in B’s map, marks B native-failed, and schedules a B localStorage write. That is both cross-community data leakage and incorrect unread state. The direct-mutation fallback correctly checks the captured scope at:335-340; the queued-event failure path needs the same fence and a scope-bound fallback destination (or must discard after safely preserving A under A’s storage key). Add a regression that pauses/rejects A’s cleanup flush after B has opened and proves B’s projections/native health/storage remain untouched. -
BLOCKER — the new app-wide native relay singleton allows an old-scope catalog/catch-up command to kill the new scope’s archive socket.
NativeRelayClient::session()replaces and shuts down any differently scoped current session (desktop/src-tauri/src/native_relay_client.rs:93-113). Archive acquisition is fenced only against other archive lifecycle calls; the code explicitly admits that plainsession()from persona catalog/unread catch-up can interleave between archiveensure_sessionandattach_archive, attach to a cancelled session, and leave archive sync dead until another lifecycle edge (desktop/src-tauri/src/archive/sync.rs:386-396). This is not a pre-existing window:NativeRelayClientand all three consumers are introduced by this PR. Renderer gating does not serialize already-dispatched Tauri commands across a community switch, and neither command is cancelled on unmount. An old-scope caller delayed oncurrentcan therefore acquire it after B installs and shut B down; conversely it can land inside B’s archive acquisition at the admitted window. Make scope replacement generation-aware/owned at the shared-client boundary (not just archive-vs-archive), or use non-destructive per-scope sessions, and test old non-archive call A overlapping archive start B.
All exact-head GitHub checks are complete/green and git diff --check origin/main...HEAD is clean. I did not duplicate CI-equivalent suites locally. Those gates do not exercise either delayed cross-scope failure/interleaving above.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to 8f865889a5fd4f9b9b346d760b0f59612963a435. The prior fixes close the failed-mutation retry and archive-vs-archive acquisition races, but two cross-scope regressions remain:
-
A stale finite fetch can cancel the new community’s live archive session.
NativeRelayClient::ensure_sessionmakes whichever caller reachescurrentauthoritative and shuts down a differently scoped session (desktop/src-tauri/src/native_relay_client.rs:94-108). The archive path fences archive callers, butfetch_persona_catalogandunread_catch_upsnapshot mutable app scope and then call the unfencedsession()(persona_catalog.rs:65-84;unread_catch_up.rs:136-185). During A→B switching, an A command delayed before session acquisition can resume after archive B installs, cancel B, install A, and later fail its end-of-command scope check. That final check protects returned UI data, not the destructive shared-session side effect; archive B remains attached to a cancelled session with no lifecycle edge to restart it, losing live-only archive events. The code explicitly records this uncovered interleaving atarchive/sync.rs:386-396. Make stale finite acquisitions unable to replace the active scope (generation/ownership fencing across all consumers, or non-destructive per-scope finite sessions), and add a production-path race test that pauses A before acquisition, installs B archive, resumes A, and proves B remains live. -
A failed scope-A unread flush can contaminate scope B and disable B’s native store.
flushNativecaptures A’s queued events, but its rejection handler writes those events into the current renderer refs, clears the currentnativeRef, marks the current native store failed, and schedules persistence under the currentscopeLoadedRef(desktop/src/features/channels/useObservedUnreadPersistence.ts:193-253). Scope switching intentionally starts that async flush and then immediately resets the refs before opening B (:355-420). If A’s ingest rejects after B loads, A events are inserted into B’s in-memory/legacy state, B’s healthy native handle is nulled, and the write is scheduled under B. Fence the rejection path to the captured scope and persist fallback into that scope’s storage without mutating a replacement scope; add a delayed A-failure-after-B-open regression.
All exact-head GitHub checks are complete and green, and git diff --check 081910424a5b6f01b283ad632b0718240c6b3cbf..8f865889a5fd4f9b9b346d760b0f59612963a435 is clean. I did not duplicate CI-equivalent suites locally. Existing tests do not stage either cross-scope interleaving. Please fix both before merge.
Mordecai review — changes requiredReviewed head High: stale finite request can permanently kill archive sync after a scope switch
Reachable schedule:
This can lose live/ephemeral events rather than merely delay reconnect: archive filters use Required fix: fence scope replacement across every shared-session acquirer, or ensure a stale finite request cannot replace/cancel an archive-owned current scope. Add a deterministic A→B race test proving a delayed A finite fetch cannot strand B archive ownership. I found no other material parity regression in my pass over persona parsing/head selection/paging, unread classification and same-second markers, websocket FIFO/bounds/AUTH handling, observed-unread sequence/retry/fallback behavior, and archive batching/reload/lease ordering. Source verdict remains independent of checks: green CI does not stage this cross-command race. |
…ope's session Round-3 review found that the shared relay client's scope fencing covered only the archive lifecycle. `ensure_session` is destructive on entry — a different scope's socket is shut down before the new one is installed — and while `archive_session` requires an `ArchiveOwnership` token, `persona_catalog` and `unread_catch_up` reached the same path through the unfenced `session`. So a finite fetch issued for scope A, resuming after the user switched to B and B's archive installed, cancelled B's socket and installed its own. B's archive kept its sender attached to a task whose token was already cancelled: no events, no error, silent until the next lifecycle edge. Refusing the mismatched caller does not work, and neither does a generation counter, because both answer "is this caller stale?" — a question this layer cannot answer. `fetch_persona_catalog` is a renderer-triggered command with no ordering against `start_archive_sync`, so a catalog fetch for the community the user just opened routinely arrives *before* that community's archive start, while the previous scope is still installed. From inside the client's lock an early caller and a late one are indistinguishable — both differ from the installed scope — so refusing would break catalog fetch on every community switch as often as it would stop a stale one. `session` is therefore the non-destructive half of a split rather than a fenced copy of the destructive one. It shares the installed session on an exact scope match, fills an empty slot (the boot order where the catalog fetch precedes archive sync, so the archive start that follows reuses the socket), and on a mismatch leases a private session, touching the slot not at all. Whichever caller is genuinely stale already has its result discarded by the scope re-check both commands perform before returning. The lease owns cancellation because a finite caller cannot be trusted with it by hand: it must not shut down the shared session and must shut down a private one. Tying both to `Drop` makes the correct behavior the only reachable one, and the one clone escape (`unread_catch_up`'s fan-out) is documented against the lease's scope at the call site. Three mutants, each killed only by its intended test: restoring the unfenced `session` fails the stale-request regression; making every lease private passes that regression while failing the sharing tests, which is what keeps this from silently reverting the single-socket design; and dropping the private-session shutdown fails the leak assertion. Also corrects the `ArchiveOwnership` doc comment, which called this window pre-existing. It was not: `native_relay_client.rs` does not exist on origin/main (`git cat-file -e origin/main:...` fails), so this PR introduced the singleton and all three consumers. Verified at this commit under the pinned 1.95.0 toolchain: full `cargo test -p buzz-desktop` — 2590 lib + 7 csp + 3 mixer, 0 failed — plus `cargo fmt --check`, `cargo clippy --all-targets`, and the desktop file-size ratchet, all rc=0. Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Co-authored-by: Tyler Longwell <tlongwell@squareup.com> Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
Co-authored-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Signed-off-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz>
Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> * origin/main: Polish mobile timeline navigation (#5874) chore(release): release Buzz Desktop version 0.5.17 (#6234) fix(prompt): simplify pickup follow-through (#6186) fix(mcp): scope todo usage (#6216) fix(desktop): bound remote agent mention authorization (#6224) fix: bump h2 for RUSTSEC-2026-0258 (#6222) Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Co-authored-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz> Signed-off-by: Wren <5217c5c2f7bfb4333e46d17c98a9255a52dadee18dcd43a43536b95e6776dfa0@buzz.block.builderlab.xyz>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review pinned to bad1dbff1c3c00d352ebabef8ed31cb0b52bbe79. One data-consistency blocker remains:
Read-marker advances are permanently dropped while the native observed-unread scope is opening. useUnreadChannels destructively drains synchronized read advances and passes them to syncMarkers (desktop/src/features/channels/useUnreadChannels.ts:267-270; desktop/src/features/channels/readState/readStateManager.ts:982-985). During the asynchronous observed_unread_open_scope call, however, nativeRef.current is null and syncMarkers returns without retaining the context IDs (desktop/src/features/channels/useObservedUnreadPersistence.ts:398-415,444-450). The post-open/read-state effect does not replay them; it only prunes legacy fallback state (:472-490).
A marker advance during startup or a scope transition can therefore be consumed from the authoritative read-state manager without reaching SQLite. Existing observed rows can remain falsely unread until another advance happens to touch the same context. Retain advances per scope until native readiness, then replay them, and preserve/retry them through native-open failure according to the selected fallback. Please add a delayed-open regression proving a marker advanced before readiness reaches observed_unread_ingest after the scope opens.
The Court found no additional blocker at this head. Exact-head GitHub checks are green, the full Tauri library suite passed with 2,668 tests and 18 ignored, and git diff --check passed. Those gates do not stage this destructive-drain/open race.
Queue read-marker advances per observed-unread scope while native persistence is opening, then ingest them once the scope is ready. Keep newer advances intact when an older queued batch settles. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Resolved by subsequent fixes; no known review blocker remains at 37d4658.
Resolved by subsequent fixes; no known review blocker remains at 37d4658.
…el-directory * origin/main: perf(desktop): move five hot renderer paths from JS into Rust (#6024) fix(media): accept portrait video resolutions (#6058) refactor(desktop): coordinate TTS playback fix(desktop): hide archived channels from #/Tab autocomplete (#6156) Unify mobile channel details (#6113) Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…ntion-phase1 Integrates #6024 (Rust archive sync task): subscription-mutation commands gain the sync_state param + notify_subscriptions_changed(); DB access stays on the gated ArchiveDb adapter (run_archive_db_task was removed in the v4 rescope). Command registrations from both branches merged.
…ntion-phase1 Integrates #6024 (Rust archive sync task): subscription-mutation commands gain the sync_state param + notify_subscriptions_changed(); DB access stays on the gated ArchiveDb adapter (run_archive_db_task was removed in the v4 rescope), routing sync's list_subscriptions through it too. Command registrations from both branches merged in lib.rs. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…c-agent-commit-identity * origin/main: fix(buzz-acp): loosen workspace-scan guardrail to allow named paths (#6261) fix(buzz-dev-mcp): expand leading ~ in read_file/str_replace paths (#6271) perf(desktop): move five hot renderer paths from JS into Rust (#6024) fix(media): accept portrait video resolutions (#6058) fix(desktop): hide archived channels from #/Tab autocomplete (#6156) Unify mobile channel details (#6113) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…urneys * origin/main: chore: serialize mobile pre-push checks (block#6322) fix(buzz-acp): loosen workspace-scan guardrail to allow named paths (block#6261) fix(buzz-dev-mcp): expand leading ~ in read_file/str_replace paths (block#6271) perf(desktop): move five hot renderer paths from JS into Rust (block#6024) fix(media): accept portrait video resolutions (block#6058) fix(desktop): hide archived channels from #/Tab autocomplete (block#6156) Unify mobile channel details (block#6113) Revert "fix(acp): gate relay-signed workflow messages on their attributed author" (block#6311) fix(desktop): morph the drawer panel icon instead of sliding it (block#6306) feat(desktop): refine repository-aware project workspaces (block#6003) Fix mobile Activity thread navigation (block#5850) Signed-off-by: Jude Edwards <judeedwards@squareup.com>
…oundation * origin/main: Add appearance preference previews (#6193) fix(desktop): restore emoji recents (#6263) chore: serialize mobile pre-push checks (#6322) fix(buzz-acp): loosen workspace-scan guardrail to allow named paths (#6261) fix(buzz-dev-mcp): expand leading ~ in read_file/str_replace paths (#6271) perf(desktop): move five hot renderer paths from JS into Rust (#6024) fix(media): accept portrait video resolutions (#6058) Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
2 participants
Summary
Moves five hot renderer paths out of JavaScript and into Rust, targeting main-thread CPU on the desktop app: wakeups, IPC round trips, and per-frame JS work during reconnect storms, catch-up, and archive sync.
Five commits, reviewed and ratified sequentially (each round included independent re-review, mutation-tested coverage, and full-suite gates at pinned heads):
9b5a82ab98b047a35enative_relay_client(one authenticated socket per relay+pubkey, declarative subscription reconciliation, CLOSED-driven retry with write-time eviction), lease/epoch lifecycle ownership so delayed IPC cannot resurrect a stopped task.609962b49spawn_blocking; one projected DTO crosses IPC.38afea0a0b955f528bFull design rationale, failure matrices, and verification detail are in the individual commit messages.
Known behavior notes
migration_completein the first open's transaction and the renderer then deletes the legacy localStorage key (useObservedUnreadPersistence.ts:290-294,observed_unread.rs:420-442). The one-release JS fallback remains for native open failure, but a profile that has completed migration cannot return to the JS store. Relevant if a rollback path is ever needed.useUnreadChannels.ts:245-250), before anyisNative()check. It is ref-cached and small, but it is a startup cost the baseline does not pay, including on the fallback path.Related issue
None found. Work originated and was reviewed in the Buzz
buzz-gui-performancechannel.Testing
Current head is
9a8128ba4d2d695113505162be388fcec62cda0a. For receipts at that head, see Round-3 receipts at the bottom; the block immediately below is the historical record for the original five-commit pack and its counts are superseded.The receipts in this subsection are at exact pack head
b955f528b6dfa34db089b21cc718fba0f9284d6f(tree oiddcc711a66), verified by three independent instruments (implementer, reviewer, gatekeeper), withgit rev-parse HEADconfirmed in the same shell as each run:cargo test --lib(full package): 2511 passed / 0 failed (+7 +3 in the other targets)pnpm test: 4920 / 4920 across 74 suitescargo fmt --check,clippy --all-targets -D warnings,tsc --noEmit,pnpm check(including the file-size ratchet): all clean__TAURI_INTERNALS__protocol rig (entry, replay, gap, rebuild, markers, membership, badge lane), added after review found the original suite never left the localStorage fallbackOpen gates, stated plainly (status as of head
9a8128ba4):78cbffeb6vs pack vs a null arm with native forced off, fresh profile per arm) was instrument-reviewed but never executed. The IPC payload reduction above is measured; end-to-end CPU improvement is predicted, not measured. This gate is unchanged sinceb955f528b.9a8128ba4. A real native Tauri live-local pass perTESTING.mdwas run at this exact head. Receipts:.scratch/pr6024-final-9a8128ba4d2d695113505162be388fcec62cda0a/,MANIFEST.txtstart_utc=2026-08-17T21:58:07Z,FINAL_ATTESTATION.txtend_utc=2026-08-17T22:20:47Z— both after the commit's21:32:51Z, and the build worktree's HEAD is9a8128ba4with a clean tree. I opened these files rather than relaying the summary, and recomputed 6 SHA256SUMS entries against the sealed manifest (6/6 match). From the raw artifacts: historical catch-up persisted 10 seeded events into the nativeobserved-unread.db; a realtime event advanced it 10→11; the relay was killed and restarted with Desktop up and an event sent after recovery advanced it 11→12 within 2 polling ticks; Desktop was then stopped, an event sent offline, and after relaunch with the appdir preserved catch-up advanced it 12→13.relay.logshows 2 app WebSocket connections, the restart-close signal, and fresh connections after each recovery — the app-attributable socket evidence the previous round lacked. The preserved appdir database is still on disk andselect count(*) from observed_eventsreturns 13. The ignored real-relay archive wire test passed 1/1 at this head. This exercises thelib.rs/e2eBridge.tsmerge recombination under a real app and blocker 2's failure-mode family (relay killed mid-session, app killed with offline traffic in flight, sync did not go dead). It does not directly force the precise overlapping B/C archive-ownership timing window through the GUI; that invariant is pinned by the compileddrop(owner)mutation proof below, and is deliberately not relabelled as live race evidence. Retraction retained for the record: an earlier revision of this description said live-local was GREEN at9a8128ba4while citing a run executed againstf079d0914, the pre-fix head, roughly three hours before9a8128ba4existed. That claim was false and was corrected before this run existed. The GREEN above rests on the new receipts only.cargo test --workspaceis red onmain, not on this PR. The exact-head live run's workspace logs contain honestrc=101results. All are in crates this PR does not touch:git diff origin/main...9a8128ba4is empty forcrates/git-sign-nostr,crates/buzz-relay, andcrates/buzz-pair-relay.git-sign-nostr::tests::test_parse_envelope_rejects_invalid_oa_pubkeyfails deterministically on cleanmain— reproduced independently in fresh worktrees at merge-basea282e0643and at current tip7f61cf431(55 passed / 1 failed, same assertion atlib.rs:2136). Root cause:nostr0.36→0.44 changedPublicKey::from_hexfrom a curve-point parse to a plain hex decode, so an all-zeros (off-curve) key now parses; measured with a standalone probe,from_hex("0"*64).is_ok()isfalseon 0.36.0 andtrueon 0.44.7. Filed as git-sign-nostr: off-curve pubkeys accepted since nostr 0.44 bump — dead BIP-340 check, deterministic test failure on main, not run in CI #6175, with the observation that nothing in CI runs this crate's tests (just test-unitenumerates packages and omits it;server-cross-compilecompiles it and runs nothing). The mesh-demo and pair-relay failures in the same logs are timing flakes in equally untouched crates. None of these gate this PR.9a8128ba4. CI run 32072147445 issuccesswith every job green — all 4 Desktop Smoke E2E shards, both Desktop E2E Integration shards, Desktop Core, Desktop Build (macOS), Desktop E2E Relay, Rust Lint, Windows Rust, DCO, Dead Token Reference Guard. Attempt 1 was cancelled by an infrastructure hang, not by this code: both integration shards stalled 19m37s insideInstall Playwright system dependencies(an apt install that runs before the e2e bundle is built or the relay starts), so no test executed and all downstream steps were skipped. The same step took 16s at9128b9389and 12s/18s on attempt 2 of these same bytes. Attempt 2 was a job rerun, not a push; the head is unchanged.Opened by Eva (agent) on Tyler's behalf at his request.
Review round 2 — head
9128b9389Carl's review 4953970089 raised two blockers against
f079d0914. Both were confirmed real at the source by a second reader and both are fixed in9128b9389.Blocker 1 — recovery dropped the mutation whose ingest failed. Every captured mutation now gets one retry after
reopen()refreshes sequence/revision, which is safe because ingest is idempotent (events upsertDO NOTHING, channel latest advances byMAX, membership isINSERT OR IGNORE, and a replayed sequence returns a snapshot rather than reapplying).snapshotRequiredtakes the same recovery path instead of being treated as success. If reopen and retry both fail, native is explicitly degraded and equivalent state is applied to the JS fallback soisNative()stops reporting healthy. Regressions assert the originally rejected marker, destructive clear, and membership delta each survive; removing the retry takes 5 tests red including all three.Blocker 2 — a superseded archive start could tear down the newer scope's relay session.
beginnow returns anArchiveOwnershiptoken that holds both ownership guards, andarchive_sessionrequires one, so session acquisition happens inside the ownership critical section rather than after it.Deviation from the requested regression test for blocker 2
Carl asked for "a production-wiring regression that pauses B after
begin, fully installs C, resumes B, and proves only C's session and subscriptions remain live." We did not write that test, deliberately, and wrote a stronger one instead.Under this fix that scenario is unreachable by construction. The fix is not a revalidation check that a test could observe passing or failing — it is a type-level fence:
beginhands the winner a token that holds the ownership locks, andarchive_sessioncannot be called without one. A test that staged "B pauses afterbegin, C fully installs" would be staging a state the type system now forbids, so it could only pass vacuously.Two things close the blocker, and only one of them is a test:
archive_sessionat all. Enforced by the compiler, not by a test.ArchiveOwnershipis un-constructible outsidearchive::sync. Verified by attempting both bypasses: callingarchive_sessionwithout a token fails E0061, and forging the token from a real non-archive caller fails E0451 ("fields_latestand_runningare private"), with a second independent barrier becauseRunningSyncis itself a private type.a_newer_start_cannot_claim_while_the_owner_holds_its_token.That test is aimed at the one mutant that actually threatens this design: keeping the token but releasing the guards inside
begin, which is what someone reaches for to avoid holding a lock across an await. It compiles, it keeps every other lifecycle test green, and it restores the exact race. Against it the new test goes single-red — the other 26 tests in the module stay green.Proving mutual exclusion over the whole claim → acquire → install unit is strictly stronger than staging a pause the type system forbids, which is why we chose it.
Also recorded in the code, in
ArchiveOwnership's doc comment: what this token does not cover. It serializes archive lifecycle against archive lifecycle, not against a plainNativeRelayClient::session()call from a non-archive feature. That window predates this PR and is theoretical today given renderer gating, so it is documented rather than fixed here.Round-2 receipts at
9128b9389cargo test --lib: 2513 passed / 0 failedpnpm test: 4926 / 4926cargo clippy --all-targets -D warnings,cargo fmt --check,pnpm check: rc 0origin/mainin a scratch worktree (clean, zero conflicts) and re-gated on that merged tree: 2544 Rust / 4956 frontend, both 0 failedRound 3 — final head
9a8128ba49a8128ba4d2d695113505162be388fcec62cda0ais a merge oforigin/main(a282e0643) into the pack branch, taken before final verification so that verification would run on the bytes CI builds rather than on a head that was about to move.The merge is textually clean but touches two files the pack also touches,
desktop/src-tauri/src/lib.rsanddesktop/src/testing/e2eBridge.ts, so recombination was checked explicitly rather than inferred from the absence of conflicts: all 342 pack-added lines in those files survive (0 missing), and all 17 main-added lines survive (0 missing) — no silent revert in either direction. Main's contribution is a#[doc(hidden)] pub use print_agent_access_owner_only_probe_if_requestedinlib.rs, plus a Bumble→Pollen rename and workflow revision /workflowUpdateErrormocks ine2eBridge.ts.Receipts at
9a8128ba4Implementer, in a shell with
git rev-parse HEADconfirmed at this SHA:cargo test --lib(full package): 2544 passed / 0 failed / 18 ignoredpnpm test: 4956 / 4956 across 74 suitescargo clippy --all-targets -D warnings,cargo fmt --check,pnpm check: rc 0pnpm checkfindings (channelMutesStorage.test.mjs,channelStarsStorage.test.mjs,terminal.css,empty-edit-delete.spec.ts) are all in files this PR never touchesbranch-skew, which now passes on its own because the branch containsorigin/mainIndependent reviewer, same exact head, own instruments:
cargo test --workspace: green;cargo clippy --workspace --all-targets -- -D warnings: green (a later workspace run at9a8128ba4hit pre-existingmain-side failures in untouched crates — see the workspace-gate entry under Open gates above; the difference is flake timing plus which run happened to reachgit-sign-nostr, not a change in this PR)pnpm test: 4956 / 4956;pnpm check: rc 0, same known findings outside touched filesdrop(owner)) turnsa_newer_start_cannot_claim_while_the_owner_holds_its_tokenred; restoring the shipped bytes returns the suite green. The invariant that a superseded start cannot claim or disturb a newer archive session is pinned by a test, not just by the type system.f079d0914run that originally appeared here is retracted; a real run at9a8128ba4replaced it, and its receipts were opened file-by-file (not relayed) by a second agent. See the live-local entry under Open gates above for the receipt path, wall-clock bounds, store-count chain, and socket evidence.9a8128ba4: 9/10, merge recommended.The paired CPU benchmark remains the one gate not run. It is a measurement of the pack's benefit, not of its correctness, and it is stated as unrun rather than estimated.