fix(send): serialize the group sender-key chain per (group, sender) - #657
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-SenderKeyName async mutexes in the cache and a SenderKeyStore::sender_key_lock trait method; callers (wacore/libsignal/features) acquire the per-sender-key lock and hold it across SKDM creation + group-message encryption to serialize sender-key chain advancement. ChangesSender-key chain serialization
Sequence DiagramsequenceDiagram
participant Client
participant Prepare as prepare_group_stanza
participant Encrypt as encrypt_group_message
participant SenderKeyStore
participant SignalCache as SignalStoreCache
Client->>Prepare: request group send
Prepare->>SenderKeyStore: sender_key_lock(sender_key_name) (await)
SenderKeyStore->>SignalCache: lookup/create Arc<async_lock::Mutex<()>>
SignalCache-->>SenderKeyStore: Arc<async_lock::Mutex<()>>
SenderKeyStore-->>Prepare: Arc<async_lock::Mutex<()>>
Prepare->>Prepare: guard.lock().await (hold)
Prepare->>Encrypt: encrypt_group_message(..., &sender_key_name, ...)
Encrypt->>SenderKeyStore: load/advance/store sender-key (under guard)
Encrypt-->>Client: encrypted group message(s)
Prepare->>Prepare: guard dropped (release)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Benchmark Results67 unchanged benchmark(s)
|
jlucaso1
marked this pull request as ready for review
May 28, 2026 22:18
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50e727728f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // JID keys ("...@g.us") can't collide with the per-device protocol | ||
| // addresses or bare user JIDs also stored in session_locks. | ||
| let group_send_mutex = self.session_lock_for(&to.to_string()).await; | ||
| let _group_send_guard = group_send_mutex.lock().await; |
There was a problem hiding this comment.
The guard acquired here is scoped only to the to.is_group() branch that builds stanza_to_send, so it is dropped before the common send_node(...) and update_sender_key_devices(...) code below runs. When the first send to a group has to distribute an SKDM, a second same-group send can acquire this lock after the first stanza is built but before the first is sent/marked, build a newer SKDM from the already-advanced sender-key iteration, and even reach the socket first; recipients that learn the chain at iteration N+1 then treat the earlier iteration N message as a duplicate/too old. To actually mirror the per-group send queue described in the comment, the same-group serialization needs to cover the network send and SKDM bookkeeping, not just encryption/stanza construction.
Useful? React with 👍 / 👎.
|
Actionable comments posted: 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jlucaso1
force-pushed
the
audit/serialize-group-sends
branch
3 times, most recently
from
May 28, 2026 23:28
a7f965d to
2956711
Compare
jlucaso1
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2956711b5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Concurrent send_message() calls to the same group raced the sender-key chain. The send path is two chain operations - SKDM creation (creates the key if absent) then skmsg encrypt (advances the chain) - over a store shared via Arc<SignalStoreCache>, with no lock between them. Two concurrent first/rotated sends could create different keys (A and B), distribute one in the SKDM and encrypt the skmsg under the other, so recipients install A and cannot decrypt a B-encrypted message; steady-state sends could likewise reuse a chain iteration. Serialize both chain operations as one unit, per (group, sender), with a lock owned by the cache: - SenderKeyStore gains sender_key_lock (default uncontended, so the in-memory test/bench stores are unchanged); SenderKeyAdapter delegates to SignalStoreCache, which hands out one shared lock per chain keyed by the same cache_key as the sender keys. - The orchestrators (prepare_group_stanza and the SignalApi group-encrypt helper) hold that lock across SKDM creation + skmsg encrypt. The cipher building blocks (encrypt_group_message, create_sender_key_distribution_*, group_encrypt) document the contract instead of self-locking, mirroring encrypt_for_devices. The SenderKeyName is now built once per send and threaded into both the SKDM creation and the encrypt instead of being rebuilt 2-3x, dropping a few per-send small-string allocations. Same-name lock lookups are allocation free (HashMap::get by &str + Arc clone); only the first send per chain allocates the lock entry. The lock does not cover query_info or the SKDM per-device fanout sessions (the latter is a separate, pre-existing per-device-session concern tracked separately).
jlucaso1
force-pushed
the
audit/serialize-group-sends
branch
from
May 28, 2026 23:42
2956711 to
d78984b
Compare
jlucaso1
changed the title
This was referenced Jul 1, 2026
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding (WA Web parity audit, area: send/encrypt)
Concurrent
send_message()calls to the same group raced the sender-key chain. The group send path performs two chain operations -- SKDM creation (create_sender_key_distribution_message_*, which creates the key if absent) and then the skmsg encrypt (encrypt_group_message, which advances the chain) -- over a store shared viaArc<SignalStoreCache>, with no serialization between them.This is reachable on any same-group concurrency (e.g. a gateway serving two requests to one group), not just flooding.
WA Web parity
WAWebSendGroupMsgJob.encryptAndSendGroupMsgruns its whole body insideWAWebSendMsgQueueMap.sendMsgQueueMap.enqueue(groupJid, ...)-- serialized per destination group JID -- because the sender-key ratchet is inherently sequential and the SKDM must match the skmsg.Change
Serialize both chain operations as one unit, per
(group, sender), with a lock owned by the cache:SenderKeyStoregainssender_key_lock(name)with a default uncontended impl, so the in-memory test/bench stores need no changes.SenderKeyAdapterdelegates toSignalStoreCache, which hands out one sharedArc<Mutex<()>>per chain (keyed by the samecache_keyas the sender keys).prepare_group_stanzaand theSignalApigroup-encrypt helper -- hold that lock across SKDM creation + skmsg encrypt. The cipher building blocks (encrypt_group_message,create_sender_key_distribution_message_for_group,group_encrypt) document the contract instead of self-locking, mirroringencrypt_for_devices.The lock is held at the orchestrator (not inside each cipher) precisely so the SKDM and the skmsg can't be split across two different keys -- locking only the encrypt would leave the preceding SKDM-create unguarded.
Performance / allocation
SenderKeyNameis now built once per send and threaded into both the SKDM creation and the encrypt, instead of being rebuilt 2-3x -- dropping a few small-string allocations per send.(group, sender)lock lookups are allocation free (HashMap::getby&strviaBorrow, thenArcclone). Only the first send per chain allocates the lock entry. The lock map is coordination-only (never time-evicted) and prunes idle entries when it grows past the cache bound.query_infoor the per-device SKDM fanout sessions; different groups and DMs are unaffected.Testing
cargo fmt,cargo clippy --all-targets -- -D warnings,cargo test -p wacore(799),cargo test -p wacore-libsignal(108) pass. Added cache tests: same name returns one shared lock (distinct names don't), and a held lock blocks a second acquire.Known separate follow-up (not in this PR)
The SKDM fan-out calls
encrypt_for_deviceswithout the per-device session locks its contract requires; two sends to different groups sharing a recipient device can still race that pairwise session during distribution. Pre-existing, cross-group, needs sorted per-device locks with its own deadlock analysis -- tracked separately.Draft -- from a WA Web parity audit; review independently.