docs: group SKDM fan-out per-device session locking (whatsapp-rust#990) - #388
Conversation
PR #990 fixed a race where the group SKDM pairwise fan-out shared no lock with the DM path's per-device session locks, letting a concurrent DM or group send silently drop a pairwise ratchet advance. Update the architecture and Signal Protocol docs, which previously stated group sends hold no per-device session locks.
Add detail on the new pairwise session lock the group SKDM fan-out acquires, alongside the existing sender-key chain lock discussion.
|
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: 53 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
|---|---|
| advanced/signal-protocol.mdx | Added a Note block explaining the new per-device session lock around the SKDM fan-out; the note contains a wrong function name (session_lock_for() instead of build_session_lock_keys()) which is inconsistent with the rest of the file. |
| concepts/architecture.mdx | Corrected the old "Group messages do not hold client-level session locks" sentence; replaced with a Note block accurately describing the new shared per-device locking; function names and lock-order explanation are consistent with the rest of the codebase docs. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GS as Group Send (prepare_group_stanza)
participant DM as DM Send
participant SL as Per-device session locks
participant CL as Chain lock (group, sender)
participant PS as Pairwise Signal sessions
Note over GS,PS: Before #990 (racy)
GS->>CL: acquire chain lock
DM->>SL: acquire session lock(device X)
GS->>PS: mutate device X ratchet (no session lock!)
DM->>PS: mutate device X ratchet (race!)
Note over PS: Silent ratchet advance drop
Note over GS,PS: After #990 (fixed)
GS->>SL: lock_device_sessions() - acquire session locks (sorted)
GS->>CL: acquire chain lock
GS->>PS: mutate SKDM target pairwise sessions (held)
GS->>CL: release chain lock
GS->>SL: release session locks
DM->>SL: acquire session lock(device X) - now serialized
%%{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 (prepare_group_stanza)
participant DM as DM Send
participant SL as Per-device session locks
participant CL as Chain lock (group, sender)
participant PS as Pairwise Signal sessions
Note over GS,PS: Before #990 (racy)
GS->>CL: acquire chain lock
DM->>SL: acquire session lock(device X)
GS->>PS: mutate device X ratchet (no session lock!)
DM->>PS: mutate device X ratchet (race!)
Note over PS: Silent ratchet advance drop
Note over GS,PS: After #990 (fixed)
GS->>SL: lock_device_sessions() - acquire session locks (sorted)
GS->>CL: acquire chain lock
GS->>PS: mutate SKDM target pairwise sessions (held)
GS->>CL: release chain lock
GS->>SL: release session locks
DM->>SL: acquire session lock(device X) - now serialized
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
advanced/signal-protocol.mdx:681
**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.
Reviews (1): Last reviewed commit: "docs: document per-device session lock a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </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. |
There was a problem hiding this comment.
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 5/5
- In
advanced/signal-protocol.mdx, the Note namessession_lock_for()even though that helper does not exist; leaving this as-is risks reader confusion and incorrect integrations when copying the DM session-locking guidance — update the reference tobuild_session_lock_keys()before merging.
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="advanced/signal-protocol.mdx">
<violation number="1" location="advanced/signal-protocol.mdx:680">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| <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. | ||
|
|
There was a problem hiding this comment.
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>
| <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. |
session_lock_for() guards the single-address decrypt path, not the DM per-device send-path locking this note describes — that section uses build_session_lock_keys() + session_mutexes_for(). Per Greptile review on #388.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
1 participant
Summary
Updates documentation to reflect whatsapp-rust#990, which fixed a race in the group send path: the SKDM pairwise fan-out mutates each target device's pairwise Signal session, but previously shared no lock with the DM path's per-device session locks — only the per-
(group, sender)sender-key chain lock, a disjoint key. A concurrent DM (or another group send) sharing a device could race that device's pairwise ratchet and silently drop an advance, permanently breaking a member's group decryption until a retry re-distributed the sender key.The fix adds a
SendContextResolver::lock_device_sessions()hook thatprepare_group_stanzanow acquires (reusing the samebuild_session_lock_keys()+session_mutexes_for()helpers the DM path uses) before taking the chain lock.Two docs pages described the pre-fix locking model and needed correcting:
concepts/architecture.mdx— the "Per-device session locks" section stated "Group messages do not hold client-level session locks", which is no longer accurate. Replaced with a note describing the shared per-device locking and why it was added.advanced/signal-protocol.mdx— added a note alongside the existing sender-key chain-lock discussion (under "Parallelized group encrypt fan-out") explaining the new pairwise session lock, its scope, and why lock ordering (session locks → chain lock) can't deadlock.No other user-facing behavior changed in #990 (it's a correctness/concurrency fix with a no-op-by-default hook), so no other docs pages needed updates. Per instructions, the
changelog/directory was left untouched.Test plan
Generated by Claude Code
Summary by cubic
Update docs to reflect per-device session locking around the group SKDM fan-out, aligning it with the DM path to prevent a race that could drop a pairwise ratchet advance and break group decryption. Adds notes in
concepts/architecture.mdxandadvanced/signal-protocol.mdxcovering the shared session locks, lock ordering (session locks → sender-key chain lock), and theSendContextResolver::lock_device_sessions()hook.Written for commit 9a5d3f3. Summary will update on new commits.