Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions advanced/signal-protocol.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,12 @@ Prior to [#807](https://github.com/oxidezap/whatsapp-rust/pull/807), a single ch
`encrypt_for_devices` is composed of two public halves: `ensure_sessions_for_devices` (network, returns `SessionPlan`) and `encrypt_for_devices_with_sessions` (CPU, consumes `SessionPlan`). The DM path calls `encrypt_for_devices` unchanged; the group path calls them separately with the chain lock taken only around the second.
</Note>

<Note>
**Per-device session lock around the SKDM fan-out (v0.6).** The chain lock above only serializes the sender-key chain — it does not cover the *pairwise* Signal sessions that `encrypt_for_devices_with_sessions` mutates for each SKDM target device. Those are the same pairwise sessions the DM path locks (see "DM per-device locking" under [Single-allocation session lock keys](#single-allocation-session-lock-keys) below) via `session_lock_for()` / `session_mutexes_for()`. Before [#990](https://github.com/oxidezap/whatsapp-rust/pull/990), the group fan-out held only the chain lock, a disjoint key, so a concurrent DM (or another group send) sharing a device could race that device's pairwise ratchet — both sides load chain index *N* and both store *N+1*, silently dropping one advance. When the lost advance carried the SKDM, that member never received the sender key and every subsequent `skmsg` stayed undecryptable for it until a retry re-distributed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Incorrect function name: session_lock_for() should be build_session_lock_keys()

The new note mentions session_lock_for() / session_mutexes_for() as the helpers the DM path uses, but session_lock_for() does not exist anywhere else in this file or in the PR description. Every other reference in this same file (lines 1402, 1429, 1440) and in the PR description consistently uses build_session_lock_keys() + session_mutexes_for(). A developer following this note to look up the implementation would search for session_lock_for() and find nothing.

Prompt To Fix With AI
This is a comment left during a code review.
Path: advanced/signal-protocol.mdx
Line: 681

Comment:
**Incorrect function name: `session_lock_for()` should be `build_session_lock_keys()`**

The new note mentions `session_lock_for()` / `session_mutexes_for()` as the helpers the DM path uses, but `session_lock_for()` does not exist anywhere else in this file or in the PR description. Every other reference in this same file (lines 1402, 1429, 1440) and in the PR description consistently uses `build_session_lock_keys()` + `session_mutexes_for()`. A developer following this note to look up the implementation would search for `session_lock_for()` and find nothing.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code


Comment on lines +680 to +682

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The Note references session_lock_for() as a DM path session locking helper, but this function does not exist. The correct function name is build_session_lock_keys() — the same pair documented everywhere else as build_session_lock_keys() + session_mutexes_for(). A reader trying to cross-reference the code with the docs would get a broken search hit.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At advanced/signal-protocol.mdx, line 680:

<comment>The Note references `session_lock_for()` as a DM path session locking helper, but this function does not exist. The correct function name is `build_session_lock_keys()` — the same pair documented everywhere else as `build_session_lock_keys()` + `session_mutexes_for()`. A reader trying to cross-reference the code with the docs would get a broken search hit.</comment>

<file context>
@@ -677,6 +677,12 @@ Prior to [#807](https://github.com/oxidezap/whatsapp-rust/pull/807), a single ch
 `encrypt_for_devices` is composed of two public halves: `ensure_sessions_for_devices` (network, returns `SessionPlan`) and `encrypt_for_devices_with_sessions` (CPU, consumes `SessionPlan`). The DM path calls `encrypt_for_devices` unchanged; the group path calls them separately with the chain lock taken only around the second.
 </Note>
 
+<Note>
+**Per-device session lock around the SKDM fan-out (v0.6).** The chain lock above only serializes the sender-key chain — it does not cover the *pairwise* Signal sessions that `encrypt_for_devices_with_sessions` mutates for each SKDM target device. Those are the same pairwise sessions the DM path locks (see "DM per-device locking" under [Single-allocation session lock keys](#single-allocation-session-lock-keys) below) via `session_lock_for()` / `session_mutexes_for()`. Before [#990](https://github.com/oxidezap/whatsapp-rust/pull/990), the group fan-out held only the chain lock, a disjoint key, so a concurrent DM (or another group send) sharing a device could race that device's pairwise ratchet — both sides load chain index *N* and both store *N+1*, silently dropping one advance. When the lost advance carried the SKDM, that member never received the sender key and every subsequent `skmsg` stayed undecryptable for it until a retry re-distributed.
+
</file context>
Suggested change
<Note>
**Per-device session lock around the SKDM fan-out (v0.6).** The chain lock above only serializes the sender-key chain — it does not cover the *pairwise* Signal sessions that `encrypt_for_devices_with_sessions` mutates for each SKDM target device. Those are the same pairwise sessions the DM path locks (see "DM per-device locking" under [Single-allocation session lock keys](#single-allocation-session-lock-keys) below) via `session_lock_for()` / `session_mutexes_for()`. Before [#990](https://github.com/oxidezap/whatsapp-rust/pull/990), the group fan-out held only the chain lock, a disjoint key, so a concurrent DM (or another group send) sharing a device could race that device's pairwise ratchet — both sides load chain index *N* and both store *N+1*, silently dropping one advance. When the lost advance carried the SKDM, that member never received the sender key and every subsequent `skmsg` stayed undecryptable for it until a retry re-distributed.
<Note>
**Per-device session lock around the SKDM fan-out (v0.6).** The chain lock above only serializes the sender-key chain — it does not cover the *pairwise* Signal sessions that `encrypt_for_devices_with_sessions` mutates for each SKDM target device. Those are the same pairwise sessions the DM path locks (see "DM per-device locking" under [Single-allocation session lock keys](#single-allocation-session-lock-keys) below) via `build_session_lock_keys()` / `session_mutexes_for()`. Before [#990](https://github.com/oxidezap/whatsapp-rust/pull/990), the group fan-out held only the chain lock, a disjoint key, so a concurrent DM (or another group send) sharing a device could race that device's pairwise ratchet — both sides load chain index *N* and both store *N+1*, silently dropping one advance. When the lost advance carried the SKDM, that member never received the sender key and every subsequent `skmsg` stayed undecryptable for it until a retry re-distributed.

`prepare_group_stanza` now acquires the SKDM targets' per-device session locks through `SendContextResolver::lock_device_sessions()` before taking the chain lock, and releases them right after the fan-out — the `skmsg` chain encrypt that follows only touches the sender-key chain, never a pairwise session. The `Client` implementation of this hook reuses `build_session_lock_keys()` + `session_mutexes_for()`, so the group and DM paths serialize on the exact same mutexes, in the same sorted order, and always acquire session locks before the chain lock — no path takes the reverse order, so this cannot deadlock. The hook defaults to a no-op, so a custom `SendContextResolver` (as used in tests and benches) is unaffected unless it opts in.
</Note>

### In-memory sender key device cache

The `SenderKeyDeviceCache` provides an in-memory caching layer over the per-device sender key tracking data stored in the database. Without this cache, every group send would require a database round-trip to load the sender key device map — the cache eliminates that overhead after the first load for each group.
Expand Down
10 changes: 9 additions & 1 deletion concepts/architecture.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,15 @@ pub(crate) session_locks: Cache<String, Arc<async_lock::Mutex<()>>>,

The DM send path resolves all known recipient devices and own companion devices from the local device registry, filters out hosted devices, excludes the sender device, and deduplicates for self-DMs — matching WA Web's `WAWebSendUserMsgJob` and `WAWebDBDeviceListFanout` behavior. The local registry is checked first; a network fetch is only triggered on a cache miss to avoid unnecessary LID-migration side effects. Session locks are acquired for all involved devices in sorted order to prevent deadlocks. The `build_session_lock_keys()` helper resolves encryption JIDs (normalizing the recipient to bare form via `to_non_ad()`), sorts by `(server, user, device)` using `cmp_for_lock_order()`, and deduplicates. The `session_mutexes_for()` helper then converts the sorted JIDs to session mutexes, reusing a single `String` buffer to avoid per-JID heap allocations.

The peer message path (single-device) acquires a single lock for the resolved encryption JID. Group messages do not hold client-level session locks — each participant device is encrypted separately inside `prepare_group_stanza`. Group stanza preparation uses `sort_dedup_by_user()` to deduplicate participants before device resolution, and `sort_dedup_by_device()` to deduplicate resolved device JIDs after LID conversion — both operate in-place on sorted `Vec<Jid>` without `HashSet` allocations.
The peer message path (single-device) acquires a single lock for the resolved encryption JID.

<Note>
**Group SKDM fan-out now shares the DM per-device session locks (v0.6).** `prepare_group_stanza`'s pairwise SKDM fan-out (`encrypt_for_devices_with_sessions`) mutates each target device's pairwise Signal session the same way the DM path does, but it was previously only covered by the per-`(group, sender)` sender-key chain lock — a disjoint key from the DM path's per-device session locks. A concurrent DM (or another group send) sharing a device could therefore race that device's pairwise ratchet: both sides load chain index *N* and both store *N+1*, silently dropping one advance. If the lost advance carried the SKDM, that member never received the sender key and every subsequent `skmsg` was undecryptable for it until a retry re-distributed.

`prepare_group_stanza` now acquires the SKDM targets' per-device session locks — via the new `SendContextResolver::lock_device_sessions()` hook, whose `Client` implementation reuses `build_session_lock_keys()` + `session_mutexes_for()` so both paths serialize on the identical mutexes — before taking the sender-key chain lock, and releases them right after the SKDM fan-out (the `skmsg` chain encrypt that follows only touches the sender-key chain, not any pairwise session). Lock order is always session locks → chain lock on every path, so this cannot introduce a deadlock. The hook defaults to a no-op, so custom `SendContextResolver` implementations (tests, benches) are unaffected unless they opt in.
</Note>

Group stanza preparation uses `sort_dedup_by_user()` to deduplicate participants before device resolution, and `sort_dedup_by_device()` to deduplicate resolved device JIDs after LID conversion — both operate in-place on sorted `Vec<Jid>` without `HashSet` allocations.

### Background Saver

Expand Down