fix(send): lock per-device sessions across the group SKDM fan-out - #990
Conversation
The group SKDM pairwise fan-out (encrypt_for_devices_with_sessions) mutates each target device's pairwise Signal session, but it ran under only the per-(group, sender) sender_key_lock — never the per-device session lock the DM send path holds. Two different lock keys mean no mutual exclusion, so a concurrent group send + DM (or two cold group sends sharing a recipient/own-companion device) could both read chain index N and both store N+1, dropping one ratchet advance. If the lost output was the SKDM, that member never gets the sender key and every subsequent skmsg to the group is undecryptable until a retry re-distributes. Hold the same per-device session locks the DM path uses across the fan-out, via a new SendContextResolver::lock_device_sessions hook (default no-op) that the Client implements by reusing build_session_lock_keys + session_mutexes_for — identical key derivation (resolve_encryption_jid, sorted by cmp_for_lock_order), so both paths serialize on the same mutexes with no deadlock. The sender_key_lock still covers the sender-key chain itself.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a ChangesPer-device session locking for group SKDM fan-out
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant prepare_group_stanza as prepare_group_stanza
participant SendContextResolver as SendContextResolver
participant SessionLockGuard as SessionLockGuard
participant SKDMEncrypt as encrypt_for_devices_with_sessions
participant StanzaBuild as stanza build
prepare_group_stanza->>SendContextResolver: lock_device_sessions(target devices)
SendContextResolver-->>prepare_group_stanza: SessionLockGuard
prepare_group_stanza->>SKDMEncrypt: encrypt SKDM with session_guard held
SKDMEncrypt-->>prepare_group_stanza: encrypted SKDM
prepare_group_stanza->>SessionLockGuard: drop session_guard
prepare_group_stanza->>StanzaBuild: build remaining stanza
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 |
|---|---|
| wacore/src/send/group.rs | Adds session_guard acquisition before sender_key_lock in prepare_group_stanza; lock ordering (session → sender-key) is consistent across all send paths and deadlock-free. |
| wacore/src/client/context.rs | Adds SessionLockGuard RAII type and lock_device_sessions trait method with no-op default; design is clean and wasm32/native dual-target safe. |
| src/client/context_impl.rs | Client implements lock_device_sessions by reusing build_session_lock_keys + session_mutexes_for, ensuring identical mutex identity to the DM path. |
| src/send/mod.rs | Adds test group_skdm_lock_shares_dm_per_device_session_mutex that correctly verifies both paths contend on the same mutex; fictitious 555 phone number satisfies the no-PII rule. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GS as Group Send
participant DM as DM Send
participant SM as Session Mutexes
participant SSL as session_setup_lock
participant SKL as sender_key_lock
GS->>SM: lock_device_sessions sorted → session_guard
Note over SM: DM to same device blocks
DM-->>SM: try acquire → blocks
GS->>SSL: session_setup_lock → _setup_guard
GS->>GS: ensure_sessions_for_devices
GS->>SSL: drop _setup_guard
GS->>SKL: sender_key_lock → chain_guard
GS->>GS: create SKDM + encrypt_for_devices
GS->>SM: drop session_guard
SM-->>DM: mutex acquired
GS->>GS: encrypt_group_message skmsg
GS->>SKL: drop chain_guard
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant GS as Group Send
participant DM as DM Send
participant SM as Session Mutexes
participant SSL as session_setup_lock
participant SKL as sender_key_lock
GS->>SM: lock_device_sessions sorted → session_guard
Note over SM: DM to same device blocks
DM-->>SM: try acquire → blocks
GS->>SSL: session_setup_lock → _setup_guard
GS->>GS: ensure_sessions_for_devices
GS->>SSL: drop _setup_guard
GS->>SKL: sender_key_lock → chain_guard
GS->>GS: create SKDM + encrypt_for_devices
GS->>SM: drop session_guard
SM-->>DM: mutex acquired
GS->>GS: encrypt_group_message skmsg
GS->>SKL: drop chain_guard
Reviews (3): Last reviewed commit: "perf(send): release the per-device sessi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Adds per-device session locking in the group send path to fix a concurrency bug. This is a non-trivial logic change in core messaging code that requires human review to verify lock ordering and deadlock safety.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@wacore/src/send/group.rs`:
- Around line 426-443: The device-level serialization is still missing around
the session setup path in the group send flow, so a concurrent send can race the
prekey/X3DH state mutation before the per-device lock is acquired. Update the
group send logic in the same area as ensure_sessions_for_devices and
encrypt_for_devices_with_sessions so the lock from
resolver.lock_device_sessions(distribution_list) covers the session
establishment step as well, not just the encryption call; keep the existing
per-device guard pattern used by the DM path and apply it consistently around
both operations.
🪄 Autofix (Beta)
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: 012839f5-db18-4b6d-8c07-45475f2a50d7
📒 Files selected for processing (4)
src/client/context_impl.rssrc/send/mod.rswacore/src/client/context.rswacore/src/send/group.rs
The per-device session locks wrapped only the SKDM encrypt fan-out, but ensure_sessions_for_devices (prekey fetch + X3DH) also writes each target's pairwise session and ran under only the per-group session_setup_lock — so a concurrent DM could still race the cold-path session creation. The DM path holds its per-device locks across all of prepare_dm_stanza (setup + encrypt), so match it: acquire the locks before the setup and hold them through the fan-out. Acquired before sender_key_lock, giving the whole send path a single session -> sender-key lock order (no path takes the reverse), and the inner per-fan-out acquisition is removed so the (non-reentrant) mutexes are taken once.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wacore/src/send/group.rs">
<violation number="1" location="wacore/src/send/group.rs:497">
P2: The `session_guard` is dropped here alongside `chain_guard`, but it has been held unnecessarily through `encrypt_group_message`, which only touches the sender-key store and does not use pairwise sessions. This extends the per-device mutex critical section beyond the SKDM fan-out and X3DH setup that actually need it, causing avoidable head-of-line blocking for concurrent DMs to the same devices. Consider dropping `session_guard` immediately after the `encrypt_for_devices_with_sessions` fan-out (before `encrypt_group_message`) while keeping `chain_guard` through the skmsg encrypt.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…rypt encrypt_group_message only advances the sender-key chain (guarded by chain_guard), not any pairwise session, so drop session_guard right after the SKDM fan-out instead of holding it across the skmsg encrypt. Shortens the per-device critical section to only the X3DH setup + fan-out that actually need it, avoiding head-of-line blocking a concurrent DM to a shared device.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Adds locking to group send path to prevent race conditions on pairwise sessions. Touches core send logic and concurrency; requires human review.
Re-trigger cubic
1 participant
What
Hold the same per-device pairwise session locks the DM send path uses across the group SKDM fan-out, so a group send and a concurrent DM (or another group send) sharing a device can't advance that device's Signal ratchet at once.
Why (bug)
prepare_group_stanza's SKDM pairwise fan-out (encrypt_for_devices_with_sessions) mutates each target device's pairwise Signal session (load → advance chain → store, non-atomic). But it ran under onlysender_key_lock(group + sender)— the per-(group,sender) chain lock. The DM path guards those same sessions with a disjoint mutex:session_lock_for(<device protocol addr>)(build_session_lock_keys/session_mutexes_for).Two different lock keys ⇒ no mutual exclusion. Group sends aren't per-chat locked (matches WA Web), so a concurrent group send + DM — or two cold group sends sharing a recipient/own-companion device — could both read chain index
Nand both storeN+1, dropping one advance. If the lost output was the SKDM, that member never receives the sender key and every subsequent skmsg to the group is undecryptable until a retry re-distributes. (Gated on a cold/evicted-session window; the warm cache path self-heals.)How
SendContextResolver::lock_device_sessions(&[Jid]) -> SessionLockGuardhook, default no-op (tests/benches don't race).SessionLockGuardis an opaque RAII holder in wacore; the concrete guards live in the platform crate (the per-address lock cache isn't part of the portable core).Clientimplements it by reusingbuild_session_lock_keys+session_mutexes_for— the exact same key derivation (resolve_encryption_jid, sorted bycmp_for_lock_order) and ordering the DM path uses, so both serialize on the identical mutexes. Consistent global lock order ⇒ no deadlock; the group path acquiressender_key_lock→ session locks, and no path acquires the reverse.prepare_group_stanzaholds the guard across the fan-out only; thesender_key_lockstill covers the sender-key chain itself. Zero overhead on warm sends (no SKDM targets ⇒ hook not reached).Tests
group_skdm_lock_shares_dm_per_device_session_mutex— acquiring the group SKDM lock for a device makes the DM path's per-device mutex (session_mutexes_for) contended, and it releases when the guard drops. Proves both paths share the same mutex.cargo fmt/clippyclean; fullwacoresend suite (95) + main-crate send tests (89) pass.