perf(client): hoist write-once state out of async mutexes - #1227
Conversation
Six pieces of client state sat behind an async lock whose critical section never awaited, and most of them were written once and then only read. group_cache and app_state_processor are lazy-init cells that nothing ever resets: the only writes are the initial None and the getter's fill, and the reconnect cleanup clears the processor's key cache in place rather than replacing it. Both become std::sync::OnceLock, so the group send path reads them with an atomic load and no Arc clone (the getters now hand back &Arc). noise_socket is replaced per connection, so it stays a cell, but every one of its four critical sections is a clone or a store. A std::sync::Mutex makes it a compile error to hold that guard across an await on the send path, which is what send_node has always relied on by convention. pending_device_sync and presence_subscriptions were async only because of the lock; their methods become sync. The per-JID re-check in the presence resubscribe loop stays: it covers an unsubscribe that lands after the snapshot was taken, and with a sync lock it is cheap. chatstate_handlers becomes a copy-on-write Arc<[_]> behind a sync RwLock, guarded by an atomic count the way node_waiters already is, so the default (no handler registered) neither takes the lock nor builds the ChatStateEvent that only a handler would read. BREAKING: Client::register_chatstate_handler is no longer async; drop the .await. Client::group_cache changes type from Mutex<Option<Arc<GroupCache>>> to OnceLock<Arc<GroupCache>>.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe client replaces selected async mutexes with synchronous locks and ChangesClient synchronization refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Presence
participant Transport
Client->>Presence: read tracked JIDs
Presence-->>Client: return snapshot
Client->>Presence: recheck JID
Presence->>Transport: send resubscription
Client->>Presence: remove unsubscribed JID
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/client.rs | Changes shared-state field types and introduces the chat-state handler count used by the dispatch fast path. |
| src/client/accessors.rs | Makes group-cache initialization synchronous and reads new synchronous state in memory reporting. |
| src/client/app_state.rs | Converts the app-state processor to write-once initialization while retaining its existing backend and runtime dependencies. |
| src/client/lifecycle.rs | Initializes the new lock types and preserves per-connection Noise-socket replacement and app-state key-cache cleanup. |
| src/client/messaging.rs | Uses synchronous Noise-socket access and copy-on-write chat-state handler publication. |
| src/features/presence.rs | Converts subscription tracking to synchronous locking while retaining per-JID checks around the awaited token lookup. |
| src/pending_device_sync.rs | Replaces the async set lock with a synchronous mutex without changing add, drain, or clear semantics. |
| src/client/tests.rs | Adds lifecycle and concurrency coverage for write-once cells and chat-state dispatch behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Caller[Client callers] --> Group[Group cache OnceLock]
Caller --> AppState[App-state processor OnceLock]
Caller --> Noise[Noise socket sync Mutex]
Caller --> Presence[Presence subscriptions sync Mutex]
Caller --> DeviceSync[Pending device sync Mutex]
Register[Register chat-state handler] --> Snapshot[Copy-on-write handler snapshot]
Snapshot --> Count[Atomic handler count]
Count --> Dispatch{Count nonzero?}
Dispatch -- Yes --> Snapshot
Dispatch -- No --> Skip[Skip event construction]
Reviews (6): Last reviewed commit: "fix(presence): re-read tracking after th..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Sixteen spawned readers do not necessarily overlap: the first can finish initializing the cell before the last is spawned, in which case the test observes a warm read and never exercises the initializer path it exists for. A start barrier holds every reader until all sixteen are ready.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
An async barrier releases sixteen tasks onto four worker threads, so at most four are ever inside the initializer together and the rest arrive to a warm cell. Both getters are synchronous now, so the readers can be OS threads behind a blocking barrier instead, which puts all sixteen at the cell at once.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Pure lock-type swap (OnceLock, sync Mutex, RwLock) for six state fields whose critical sections are trivially sync; no behavior change, verified by tests and benchmarks, no operational or security tradeoff introduced.
Re-trigger cubic
|
@claude fix the conflicts |
# Conflicts: # src/client/lifecycle.rs
|
Claude finished @jlucaso1's task in 12m 52s —— View job Merge resolved locally — push blocked by workflow permissions
What the merge doesOrigin/main's #1226 refactored The commit ( To land itEither grant the workflow git fetch origin main
git merge origin/main
# resolve the same overlapping files as above; ns-level, mechanical
git pushI cannot modify workflow files due to GitHub App permissions — see the FAQ. Also: I could not run |
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/presence.rs (1)
152-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-check tracking immediately before
send_node.
build_subscription_node()awaitslookup_tc_token_for_jid(). Anunsubscribe()can send its unsubscribe stanza and removejidwhile that await is pending. This method can then send a subscribe stanza after the unsubscribe and leave the remote subscription active while local tracking is inactive.Proposed fix
let node = self.build_subscription_node(jid).await; + if !self.client.is_presence_subscription_tracked(jid) { + return Ok(()); + } self.client.send_node(node).await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/presence.rs` around lines 152 - 158, Update re_subscribe_when_active to re-check is_presence_subscription_tracked(jid) after build_subscription_node(jid).await and immediately before send_node; return Ok(()) when tracking was removed during the await, otherwise preserve the existing send behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/presence.rs`:
- Around line 539-540: Update the JID literals in the relevant presence test to
use valid fictitious NANP numbers with a real non-555 NPA and subscriber numbers
in the 555-0100 through 555-0199 range, such as 12025550111 and 12025550122;
keep the existing parsing and assertions unchanged.
In `@src/usync.rs`:
- Line 516: Update the call to PendingDeviceSync::add in the surrounding sync
logic to explicitly bind its bool result to _, preserving concurrent re-enqueue
behavior while satisfying unused_must_use and workspace Clippy with -D warnings.
---
Outside diff comments:
In `@src/features/presence.rs`:
- Around line 152-158: Update re_subscribe_when_active to re-check
is_presence_subscription_tracked(jid) after build_subscription_node(jid).await
and immediately before send_node; return Ok(()) when tracking was removed during
the await, otherwise preserve the existing send behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 38a3384e-d791-4711-b58c-93d4e553e321
📒 Files selected for processing (21)
src/client.rssrc/client/accessors.rssrc/client/adapters.rssrc/client/app_state.rssrc/client/device_registry.rssrc/client/lifecycle.rssrc/client/messaging.rssrc/client/node_io.rssrc/client/tests.rssrc/client/voip.rssrc/features/chat_actions.rssrc/features/groups.rssrc/features/presence.rssrc/handlers/call.rssrc/message/receive.rssrc/message/tests.rssrc/pending_device_sync.rssrc/retry.rssrc/test_utils.rssrc/usync.rssrc/voip/facade.rs
💤 Files with no reviewable changes (1)
- src/client/node_io.rs
`build_subscription_node` awaits a tctoken lookup. An `unsubscribe` landing in that window has already sent its own stanza, so subscribing afterwards leaves the peer subscribed while we no longer track it. Re-read before the send. This narrows the window rather than closing it — `send_node` awaits too — but the lookup is the wide half and the re-read is an uncontended lock. Also moves the test JIDs onto the fictitious NANP form used elsewhere in the repo (real NPA, 555-01xx subscriber) instead of a 555 area code.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/presence.rs`:
- Around line 158-166: Serialize unsubscribe and re-subscribe operations in the
presence flow around re_subscribe_when_active and the unsubscribe handler,
holding the operation ordering through send completion so an unsubscribe cannot
overtake a checked re-subscribe. Remove reliance on the final synchronous
tracking check before send_node; use the serialized mechanism to make the intent
and transport send linearizable. Add a regression test that pauses node
construction or send handoff after the final check, completes unsubscribe, and
verifies no stale subscribe reaches the transport.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 81724c0a-ad48-4d16-ba4a-fb28753f9363
📒 Files selected for processing (1)
src/features/presence.rs
2 participants
Summary
Six pieces of client state shared one shape: an
async_lockguard whose critical section never awaits, and which in most cases is written once and thereafter only read. That shape costs a future poll and a waiter-list touch per read, and it hides the one thing you actually want the compiler to enforce, which is that nobody holds the guard across an.await.I checked each claim before touching it, and the rule I applied was that a cell only becomes a
OnceLockif it is provably never rewritten.group_cacheandapp_state_processorqualify: their only writes are the initialNoneat construction and the getter's fill, and the reconnect cleanup clears the app-state processor's key cache in place rather than swapping the processor.noise_socketdoes not qualify, since it is replaced per connection, so it only drops to astd::sync::Mutex.All of this is easy to undo: every change is a lock-type swap plus the
.awaitremovals that follow from it, with no ownership moved, no channel introduced and no init relocated. Nothing here changes what goes on the wire.One note on a claim in the audit that I could not confirm, because it changes the reasoning rather than the outcome:
std::sync::OnceLock::get_or_initdoes not have a benign construct-and-discard race. It blocks concurrent initializers and runs the closure exactly once, so the "is construction side-effect free" question never arises. It is free anyway (AppStateProcessor::newis twoArcclones and an empty map;build_typed_ttlis a cache builder), but the guarantee is stronger than the audit assumed and there is a test for it.Changes
group_cache->std::sync::OnceLock<Arc<GroupCache>>. Confirmed by grep that no path resets it,cleanup_connection_stateincluded.get_group_cacheis no longerasyncand now returns&Arc<GroupCache>, so the group send path reads it with an atomic load and noArcclone at all (it used to be forced to clone under the guard).memory_reportuses.get(), so producing a report can no longer be the thing that builds the cache.app_state_processor->std::sync::OnceLock<Arc<AppStateProcessor>>. Same shape;lifecycle.rscallsclear_key_cache()on the processor and never replaces it.get_app_state_processorispub(crate), so the signature change is internal.noise_socket->std::sync::Mutex. Rewritten per connection, so aOnceLockis out. All four critical sections are a clone or a store, and the non-Sendguard now makes holding one across an.awaita compile error on the send path, which everysend_nodegoes through. The read loop already hoisted this lock out of the per-frame path andis_connected()already avoids probing it; the send path never got the same treatment.pending_device_sync->std::sync::Mutex,add/take_all/clearsync. They wereasync fnonly because of the lock.presence_subscriptions->std::sync::Mutex, helpers sync. The N+1 re-acquisition inresubscribe_presence_subscriptionsis kept deliberately: the per-JID re-check covers anunsubscribethat lands after the snapshot was taken, which is real behaviour and not waste. There is a test for exactly that interleaving, and I confirmed it fails if the re-check is removed.chatstate_handlers->std::sync::RwLock<Arc<[ChatStateHandler]>>with copy-on-write registration, guarded by an atomic count in the style of the existingnode_waiters/node_waiter_countpair. Dispatch used to clone the wholeVecand build aChatStateEventunconditionally, even in the default case of zero handlers; now zero handlers means neither the lock nor the event build. TheEvent::ChatPresencebus dispatch above it is untouched.Breaking:
Client::register_chatstate_handleris no longerasync. Migration: drop the.await(client.register_chatstate_handler(h)).Breaking: the public
Client::group_cachefield changes type fromMutex<Option<Arc<GroupCache>>>toOnceLock<Arc<GroupCache>>. Migration:client.group_cache.lock().await.clone()becomesclient.group_cache.get().cloned().Cost
Lock acquisitions per send, counted with temporary atomic counters on the getters and a warm cached group read plus one outgoing stanza (
GROUP_CACHE_ACQ=1 NOISE_SOCKET_ACQ=1):Arcclone per group send eliminated outright (group_cache-> atomic load, and the getter returns a reference so callers no longer clone).std::sync::Mutexacquisition (noise_socket; still one acquisition, just a cheaper one).Timing is in-process and synthetic: a pinned (
taskset -c 0-3) release-build harness, 400k iterations per case, baseline and patch in the same binary, 3 rounds, medians below. The baselines are the old shapes holding the sameArcs so the clone is paid identically on both sides, and the control isclient.transport— anasync_lock::Mutex<Option<Arc<_>>>this PR does not touch, i.e. literally whatnoise_socketused to be. I could not usedivanhere: both getters arepub(crate)and abenches/target is a separate crate, so it cannot reach them. The harness is not part of this PR.ns/op, aggregate over all tasks:
transportasync_lock (unchanged)group_cacheasync_lockgroup_cacheOnceLock + Arc clonegroup_cacheOnceLock, no clone (what the real path does)noise_socketasync_locknoise_socketstd::sync::MutexArc::clone, no lock at allUncontended, which is the realistic per-send case: 57.9 -> 9.8 ns for
group_cacheand 57.8 -> 27.4 ns fornoise_socket.The contended columns look like a regression and I am not going to hide them, but they are not measuring the lock. A bare
Arc::cloneof a sharedArcwith no lock at all already costs 72-76 ns/op in the same loop, so most of the patched contended figure is refcount cache-line ping-pong that both shapes pay. The async baselines stay flat at ~57 ns precisely because the async mutex serializes the tight loop and keeps that cache line on one core — flatness there is an artifact of the tasks not running in parallel, not a win. TheOnceLockarm that does not clone stays at 33-36 ns even at 16 tasks, below the async baseline. And the workload itself is pathological: four cores re-reading the same cell at ~10M ops/s/core with zero work in between, which no send path approaches.So, honestly: this is single-digit-to-tens-of-nanoseconds per send in the uncontended case, and the microbenchmark cannot cleanly speak to the contended one. The argument I would actually defend the PR on is the structural one — one lock acquisition per group send gone entirely, and a guard that the compiler now refuses to let anyone hold across an
.awaiton the send path.Checked and not changed
noise_socketas aOnceLock. Dropped, as the scope anticipated:lifecycle.rssets it on connect and clears it toNoneon cleanup, so it is genuinely rewritten per connection. It got the sync-mutex treatment instead.get_or_init"benign race". Not confirmed, see Summary.std::sync::OnceLock::get_or_initblocks concurrent initializers rather than letting two construct and one discard, so no construction is ever thrown away.concurrent_first_readers_agree_on_one_instancecovers it..awaitare call adaptation only; the full lib suite passes unmodified otherwise.Validation
New tests: write-once cells return the same instance across calls and survive
cleanup_connection_state; 16 concurrent first readers agree on one instance; chatstate dispatch with zero handlers builds no event (proved with a#[cfg(test)]build counter, not just "no handler ran") and with two handlers reaches both; and a gated-transport test that lands anunsubscribeafter the resubscribe snapshot but before the loop reaches that JID, asserting it is not re-subscribed. That last one I mutation-checked: it fails (left: 2, right: 1) when the per-JID re-check is disabled.Full matrix left to CI. E2E not run — no mock server available here.
Generated by Claude Code