fix(recv): serialize the group inbound sender-key chain with a lock

[jlucaso1]

What

Acquire the per-(group, sender) sender-key chain lock around group_decrypt in the inbound process_group_enc_batch, mirroring what the 1:1 receive path already does with session_lock_for.

Why (bug)

process_group_enc_batch called group_decrypt (load → advance chain → store, non-atomic) with no lock, while the sibling 1:1 path holds session_lock_for around its decrypt. So two workers for the same (group, sender) could advance the receiving sender-key ratchet concurrently, and the last store wins — losing a chain advance and its persisted skipped-message keys.

A duplicate worker is reachable: a live ChatLane gets capacity-evicted (its worker keeps draining rx, only exiting on a connection_generation mismatch), and a later stanza for that chat misses the cache and spawns a second worker at the same generation. The global processing semaphore doesn't help — it's widened online, so the per-chat worker is the only serializer.

Result: later legitimate skmsg fail (bad-mac / NoSenderKeyState / spurious DuplicatedMessage) and retry-storm until the sender rotates an SKDM.

How

• Wrap group_decrypt in sender_key_lock(&sender_key_name).lock(). The lock is the shared per-(group, sender) mutex from the signal cache (same instance the store hands out everywhere), so two inbound workers serialize on it.
• Scoped to the ratchet mutation only — released before plaintext handling, matching the 1:1 path (which drops its session lock before handle_decrypted_plaintext).

Note: this closes the ratchet-corruption (P1) regardless of the duplicate-worker window. The eviction-spawns-a-duplicate-worker root cause (and the 1:1 FIFO-ordering symptom) is a separate concern handled elsewhere; sender_key_lock here makes the group path robust to it either way.

Tests

• group_skmsg_decrypts_under_sender_key_lock (happy) — an skmsg with an established sender key decrypts through the batch under the new lock and surfaces its content.
• group_skmsg_without_sender_key_takes_retry_path (bad) — an skmsg whose sender key was never distributed hits NoSenderKeyState under the lock and takes the retry path (one undecryptable event, no user content).
• cargo fmt / clippy clean; the 48 group/skmsg receive tests pass.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

@jlucaso1, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5e50ae39-389f-4ef9-a8e2-a2f46b8c9d66

📥 Commits

Reviewing files that changed from the base of the PR and between e8f05fa and c14ec35.

📒 Files selected for processing (2)

• src/message/receive.rs
• src/message/tests.rs

📝 Walkthrough

Walkthrough

This PR adds explicit sender-key chain locking around group message decryption in process_group_enc_batch, using a lock acquired from the sender key store before calling group_decrypt. An import of SenderKeyStore is added, and two new tests validate happy-path and undecryptable-path decrypt behavior.

Changes

Sender-key lock for group decryption

Layer / File(s)	Summary
Sender-key lock around group_decrypt src/message.rs, src/message/receive.rs	Imports SenderKeyStore and wraps group_decrypt in process_group_enc_batch with a lock guard acquired via sender_key_store.sender_key_lock(&sender_key_name), replacing the prior unsynchronized call.
Tests for locked decrypt paths src/message/tests.rs	Adds two async tests: one confirming successful decrypt/dispatch of group skmsg when the sender key was distributed, and one confirming a single UndecryptableMessage dispatch with no content when it wasn't.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• oxidezap/whatsapp-rust#109: Both standardize group sender-key handling around SenderKeyStore/SenderKeyName and group_decrypt keying.
• oxidezap/whatsapp-rust#657: Both use SenderKeyStore::sender_key_lock to serialize sender-key chain operations, one for encryption and one for decryption.
• oxidezap/whatsapp-rust#713: Both touch sender-key-chain storage semantics affecting how group_decrypt mutates the record.

Suggested reviewers: Ari4ka

Look, this is exactly the kind of fix we need — concurrency bugs in messaging are not something you move fast and break things on. Locking the sender-key chain before decrypt is the right call, non-negotiable for correctness at scale. The tests cover both the happy path and the undecryptable path, which is good, because at our scale, edge cases aren't edge cases — they're Tuesday. Ship it, but I want the lock contention monitored in production.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: serializing inbound group sender-key decrypt with a lock.
Description check	✅ Passed	The description directly explains the sender-key lock fix, why it was needed, and the added tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/fix-group-recv-sender-key-lock

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR serializes concurrent group_decrypt calls for the same (group, sender) by acquiring the per-key sender_key_lock inside process_group_enc_batch, closing a race where two coexisting chat-lane workers could non-atomically advance the same sender-key ratchet and corrupt persisted chain state.

• receive.rs: Fetches the shared Arc<Mutex<()>> for the SenderKeyName once before the payload loop and acquires/releases it around each individual group_decrypt call, matching the 1:1 path's session_lock_for pattern.
• message.rs: Adds the required SenderKeyStore trait import so the new sender_key_lock call resolves.
• tests.rs: Adds two tests — a happy-path decrypt under the lock and a missing-key retry path — both using consistent polling loops.

Confidence Score: 5/5

Safe to merge — targeted addition of a mutex guard around an existing call with no changes to decryption logic or error handling.

The lock acquisition and release boundaries are correct: the shared Arc is retrieved once per batch, acquired per ratchet advance, and released before plaintext handling, exactly mirroring the 1:1 path. The underlying sender_key_lock implementation already has tests for shared identity across callers, and the two new integration tests cover both the happy-path and missing-key error path.

No files require special attention.

Important Files Changed

Filename	Overview
src/message/receive.rs	Core fix: fetches the shared sender-key mutex once before the payload loop and acquires it per-iteration around group_decrypt, correctly serializing ratchet advances without blocking plaintext handling.
src/message.rs	Adds the SenderKeyStore trait import required to call sender_key_lock; one-line change, no logic touched.
src/message/tests.rs	Two new tests covering happy-path and no-key-error paths under the new lock; both use polling loops consistent with the existing test suite.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant WA as Worker A (group, sender)
    participant WB as Worker B (group, sender)
    participant Lock as sender_key_lock Arc Mutex
    participant Store as SenderKeyStore ratchet state

    Note over WA,WB: Both workers coexist after chat-lane eviction

    WA->>Lock: sender_key_lock(sender_key_name).await
    WB->>Lock: sender_key_lock(sender_key_name).await

    WA->>Lock: chain_lock.lock().await - guard acquired
    WB->>Lock: chain_lock.lock().await - BLOCKED

    WA->>Store: group_decrypt - load, advance, store
    Store-->>WA: plaintext

    WA->>Lock: drop _chain_guard - released
    WA->>WA: handle_decrypted_plaintext

    WB->>Lock: chain_lock.lock().await - guard acquired
    WB->>Store: group_decrypt - load, advance, store
    Store-->>WB: plaintext
    WB->>Lock: drop _chain_guard - released
    WB->>WB: handle_decrypted_plaintext

Loading

%%{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 WA as Worker A (group, sender)
    participant WB as Worker B (group, sender)
    participant Lock as sender_key_lock Arc Mutex
    participant Store as SenderKeyStore ratchet state

    Note over WA,WB: Both workers coexist after chat-lane eviction

    WA->>Lock: sender_key_lock(sender_key_name).await
    WB->>Lock: sender_key_lock(sender_key_name).await

    WA->>Lock: chain_lock.lock().await - guard acquired
    WB->>Lock: chain_lock.lock().await - BLOCKED

    WA->>Store: group_decrypt - load, advance, store
    Store-->>WA: plaintext

    WA->>Lock: drop _chain_guard - released
    WA->>WA: handle_decrypted_plaintext

    WB->>Lock: chain_lock.lock().await - guard acquired
    WB->>Store: group_decrypt - load, advance, store
    Store-->>WB: plaintext
    WB->>Lock: drop _chain_guard - released
    WB->>WB: handle_decrypted_plaintext

Loading

_{Reviews (2): Last reviewed commit: "perf(recv): hoist the group sender-key l..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 2

🤖 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/message/receive.rs`:
- Around line 1228-1235: Hoist the sender-key lock lookup out of the payload
loop in receive.rs: `sender_key_name` is already ثابت for the batch, so avoid
calling `adapter.sender_key_store.sender_key_lock(&sender_key_name).await` on
every iteration. Fetch the `Arc<Mutex<()>>` once before the loop in the same
`receive`/payload-processing flow, then only call `.lock().await` inside the
loop before `group_decrypt`, keeping the existing `group_decrypt` and
`sender_key_store` usage unchanged.

In `@src/message/tests.rs`:
- Around line 6673-6678: The test in message tests uses a fixed
tokio::time::sleep before asserting decrypted group content, which can be flaky
under load. Update this happy-path case to use the same polling/retry approach
as the undecryptable-event test nearby: repeatedly check message_texts_for_id
for the expected "hello group" result until it appears or a timeout is reached.
Keep the assertion anchored around the existing message_texts_for_id helper and
the chain-lock decrypt behavior so the test waits for dispatch completion
instead of assuming a single 50ms delay is enough.

🪄 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: 9cd7844f-b2e6-4a27-80e4-252484232c5b

📥 Commits

Reviewing files that changed from the base of the PR and between 8382d3d and e8f05fa.

📒 Files selected for processing (3)

• src/message.rs
• src/message/receive.rs
• src/message/tests.rs

[cubic-dev-ai]

2 issues found across 3 files

Confidence score: 4/5

• In src/message/tests.rs, the fixed 50ms sleep can make the async event assertion flaky, so merges could intermittently fail CI even when runtime behavior is correct — switch to polling for the expected text with a bounded timeout before merging to de-risk test stability.
• In src/message/receive.rs, repeatedly calling sender_key_lock(&sender_key_name).await inside the payload loop adds avoidable async lookup overhead, which could degrade batch processing efficiency as payload counts grow — hoist the lock lookup outside the loop (or track as a near-term follow-up).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	10.78 MiB	10.78 MiB	+1.00 KiB (+0.01%) 🔺
bin .text	8.78 MiB	8.79 MiB	+960 B (+0.01%) 🔺
bin allocated (text+data+bss)	10.78 MiB	10.78 MiB	+3.96 KiB (+0.04%) 🔺
llvm-lines wacore	504,289	504,289	0
llvm-lines wacore copies	17,275	17,275	0
llvm-lines whatsapp-rust lib	751,079	751,182	+103 (+0.01%) 🔺
llvm-lines whatsapp-rust lib copies	24,487	24,487	0
deps crates (Cargo.lock)	466	466	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.61 MiB	1.61 MiB	+544 B (+0.03%) 🔺
.text wacore	530.46 KiB	530.46 KiB	0
.text wacore_binary	157.49 KiB	157.49 KiB	0
.text wacore_libsignal	178.73 KiB	178.73 KiB	0
.text wacore_appstate	156.42 KiB	156.42 KiB	0
.text wacore_noise	26.05 KiB	26.05 KiB	0
.text waproto	1.60 MiB	1.60 MiB	0
.text whatsapp_rust_sqlite_storage	512.98 KiB	512.98 KiB	0
.text whatsapp_rust_tokio_transport	43.61 KiB	43.61 KiB	0
.text whatsapp_rust_ureq_http_client	10.47 KiB	10.47 KiB	0
.text std	1022.24 KiB	1022.64 KiB	+410 B (+0.04%) 🔺
.text other deps	2.95 MiB	2.95 MiB	0

Baseline: 8382d3d51 (latest main run) · Head: 1e0cc694f · Graphs

[cubic-dev-ai]

0 issues found across 2 files (changes from recent commits).

<sub>Auto-approved: Adds missing sender-key lock to prevent race in group decryption, with tests. Low-risk synchronization fix.

Re-trigger cubic</sub>