perf(recv): hold the per-sender session lock only around Signal decrypt - #983
Conversation
process_session_enc_batch held the per-sender ratchet lock across the whole batch, including handle_decrypted_plaintext (protobuf decode, SKDM/app-state/ LID/PDO/history handling, and app dispatch). None of that touches the Signal session ratchet — only message_decrypt does — so the lock only needs to cover the crypto. Buffer each decrypted plaintext during the locked decrypt loop, release the guard, then run handle_decrypted_plaintext unlocked. A concurrent stanza from the same sender (e.g. the same device across two group chats, or the detached LID-migration task) can now start decrypting while this one dispatches. This matches whatsmeow, which holds the per-sender lock only around the libsignal decrypt. Correctness is preserved: - Per-chat delivery order is owned by the serial chat-lane worker, not this guard, so releasing it early cannot reorder delivery. - The buffer drains before the function returns, so a pkmsg's SKDM is still applied before PASS 2's group (skmsg) decrypt reads the sender key. - The PN->LID migration retry helper now buffers its plaintext too and returns a small result enum, keeping multi-payload dispatch order uniform and collapsing the four verbose outcome-merge call sites into a match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116cfRpumbEv3H4qUNmtNk4
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPN→LID migration decrypt handling now returns a three-state result, buffers decrypted plaintext during batch processing, and processes deferred plaintext after releasing the session lock. ChangesMigration decrypt result and deferred dispatch
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
|---|---|
| src/message/receive.rs | Core change: decrypt loop now pushes to deferred instead of immediately calling handle_decrypted_plaintext; drop(session_guard) precedes the drain; try_pn_to_lid_migration_decrypt now takes &mut Vec<DeferredPlaintext> and returns MigrationDecryptResult; handle_decrypted_plaintext receiver changed to &Arc<Self>. All flag-merge call sites simplified to match. Logic is correct and flag semantics preserved. |
| src/message.rs | Replaces five-field MigrationDecryptOutcome struct with focused MigrationDecryptResult enum; adds DeferredPlaintext struct. Both types are clearly documented and tightly scoped to their purpose. |
| src/message/tests.rs | Removes .clone() calls on handle_decrypted_plaintext call sites (matching new &Arc<Self> receiver); adds an ignored bench_session_decrypt_throughput benchmark with warmup phase, steady-state timing, and Linux-specific memory stats via /proc/self/status. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant process_session_enc_batch
participant session_guard as session_guard (ratchet lock)
participant message_decrypt
participant deferred as deferred buffer
participant handle_decrypted_plaintext
Caller->>process_session_enc_batch: payloads, info, sender_jid
process_session_enc_batch->>session_guard: lock_arc() — acquire
loop for each payload
process_session_enc_batch->>message_decrypt: decrypt (ratchet advances)
message_decrypt-->>process_session_enc_batch: Ok(plaintext)
process_session_enc_batch->>deferred: push(DeferredPlaintext)
Note over process_session_enc_batch: outcome.decrypted = true
end
process_session_enc_batch->>session_guard: drop() — RELEASE LOCK
Note over session_guard: concurrent same-sender stanza can now acquire and decrypt
loop drain deferred
process_session_enc_batch->>handle_decrypted_plaintext: enc_type, plaintext, padding_version
handle_decrypted_plaintext-->>process_session_enc_batch: PlaintextHandleOutcome
Note over process_session_enc_batch: update dispatched / skdm_only flags
end
process_session_enc_batch-->>Caller: SessionBatchOutcome
%%{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 Caller
participant process_session_enc_batch
participant session_guard as session_guard (ratchet lock)
participant message_decrypt
participant deferred as deferred buffer
participant handle_decrypted_plaintext
Caller->>process_session_enc_batch: payloads, info, sender_jid
process_session_enc_batch->>session_guard: lock_arc() — acquire
loop for each payload
process_session_enc_batch->>message_decrypt: decrypt (ratchet advances)
message_decrypt-->>process_session_enc_batch: Ok(plaintext)
process_session_enc_batch->>deferred: push(DeferredPlaintext)
Note over process_session_enc_batch: outcome.decrypted = true
end
process_session_enc_batch->>session_guard: drop() — RELEASE LOCK
Note over session_guard: concurrent same-sender stanza can now acquire and decrypt
loop drain deferred
process_session_enc_batch->>handle_decrypted_plaintext: enc_type, plaintext, padding_version
handle_decrypted_plaintext-->>process_session_enc_batch: PlaintextHandleOutcome
Note over process_session_enc_batch: update dispatched / skdm_only flags
end
process_session_enc_batch-->>Caller: SessionBatchOutcome
Reviews (6): Last reviewed commit: "perf(recv): drop the per-payload clone i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@src/message/receive.rs`:
- Around line 741-744: Preserve the observable order for mixed success/failure
payload batches in receive_message handling. In src/message/receive.rs, update
the receive/decrypt loop around the logic that sets outcome.decrypted and pushes
into deferred so that not only plaintext successes but also undecryptable
events, retries, and nack side effects are queued in order. Then, after
drop(session_guard), drain that ordered action queue so later failures do not
run inline before earlier successful payloads are processed.
🪄 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: e0c61009-73d9-4a51-ad01-e0d6e86c3047
📒 Files selected for processing (2)
src/message.rssrc/message/receive.rs
![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: 6377263ad9
ℹ️ 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".
![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: 3/5
- In
src/message/receive.rs, the batch loop now defers successful plaintext handling but still runs failure handling inline, so a later decrypt/parse failure in a mixed batch can fire undecryptable/retry side effects before earlier successful messages are applied, creating ordering/regression risk if merged as-is. De-risk by making success/failure effects use the same phase (or explicitly preserving original ordering) and add a mixed-batch test before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Self-review cleanup: replace the positional (&'static str, Vec<u8>, u8) buffer tuple with a named DeferredPlaintext struct so the migration helper signature and each push/drain site read for themselves, and trim the two verbose block comments to their load-bearing invariant. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116cfRpumbEv3H4qUNmtNk4
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Self-review follow-up: the comment said the guard is held "across the entire batch", but the lock-scope change releases it before the plaintext drain. Say "across the decrypt loop" and note the early release. Comment only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116cfRpumbEv3H4qUNmtNk4
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message/receive.rs (1)
1542-1570: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard hand-back contract holds — good, keep it that way.
Every exit path leaves
session_guardin the state the caller expects: the earlyNotDecryptedreturns (Lines 1543, 1547) never touch the guard, and once it's cleared at Line 1552 it's re-acquired at Line 1558 before any subsequent return. That invariant is load-bearing for the next payload in the batch, so it's worth a short comment or debug-assert that the guard isSomeon return to stop a future edit from silently dropping serialization.🤖 Prompt for 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. In `@src/message/receive.rs` around lines 1542 - 1570, The guard hand-back invariant in the migration retry path of receive.rs should be made explicit so future edits do not accidentally drop serialization. In the function handling migration decrypt results, around the session_guard release/reacquire flow and the NotDecrypted early exits, add a short comment or debug_assert near the return path that makes it clear session_guard must be Some whenever control leaves this block after it has been cleared, and verify the hand-back happens in the migrate_signal_sessions_on_lid_discovery path before returning.
🤖 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.
Outside diff comments:
In `@src/message/receive.rs`:
- Around line 1542-1570: The guard hand-back invariant in the migration retry
path of receive.rs should be made explicit so future edits do not accidentally
drop serialization. In the function handling migration decrypt results, around
the session_guard release/reacquire flow and the NotDecrypted early exits, add a
short comment or debug_assert near the return path that makes it clear
session_guard must be Some whenever control leaves this block after it has been
cleared, and verify the hand-back happens in the
migrate_signal_sessions_on_lid_discovery path before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: caa8547f-a762-4513-a3ae-79b5dbcc337e
📒 Files selected for processing (1)
src/message/receive.rs
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: AI found no issues, but this optimization touches critical decryption and session lock logic in the message path. It could introduce subtle concurrency bugs that require human validation.
Re-trigger cubic
try_pn_to_lid_migration_decrypt drops and re-acquires the per-sender session guard around the migration loop; every return after the re-acquire must leave the guard Some or the caller silently loses same-sender serialization for the next batch payload. Make that load-bearing invariant explicit with a debug_assert at the re-acquire boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116cfRpumbEv3H4qUNmtNk4
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 `@src/message/receive.rs`:
- Around line 1560-1564: The debug_assert in receive-related session migration
is tautological because it checks session_guard.is_some() immediately after
assigning Some(...), so it cannot catch invariant violations. In the receive.rs
logic around the PN→LID migration path, either remove the assert and leave a
plain comment documenting the handoff, or move the check to a later point in the
same control flow where it can verify that every return path still leaves
session_guard as Some before exiting.
🪄 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: b1ed2797-0ac7-4aaf-98a2-14a667eb6d79
📒 Files selected for processing (1)
src/message/receive.rs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ng it The debug_assert added in the previous commit sat immediately after `*session_guard = Some(...)`, so it was tautological — it could never observe a missing hand-back on a later return path. Drop it and keep the invariant as a comment at the reacquire boundary (flagged by cubic and CodeRabbit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116cfRpumbEv3H4qUNmtNk4
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Modifies per-sender session lock scope in message receive path; high-risk concurrency change requires human review.
Re-trigger cubic
…ypt bench handle_decrypted_plaintext never moves self — every callee takes &self / &Arc<Self> — so take self: &Arc<Self> and drop the per-payload Arc clone at the session-drain and group-path call sites. Also add an ignored steady-state session-decrypt benchmark (bench_session_decrypt_throughput) that drives the real process_session_enc_batch against established Signal sessions; deterministic CPU/allocation profiles come from running the same binary under valgrind (callgrind/dhat). Profiling shows the decrypt path is crypto-bound (~93% message_decrypt: SHA-256 + curve25519); the batch orchestration this touches is ~0.06% of instructions, so the clone removal is a hygiene win, not a throughput change. 933 lib tests pass, clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116cfRpumbEv3H4qUNmtNk4
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Not reviewed (too large): src/message/tests.rs (~155 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Requires human review: This is a non-trivial refactor of the signal session decryption locking logic that touches core crypto paths and data integrity boundaries. Even though the AI review found no issues, changes of this nature involving lock scope, deferred processing, and concurrency risk require human review.
Re-trigger cubic
2 participants
Summary
Second of the recv post-decrypt dispatch series (follow-up to #981). This one narrows the per-sender session lock so it covers only the Signal ratchet crypto, not the downstream plaintext handling and app dispatch.
process_session_enc_batchheld the per-sender ratchet guard (session_lock_for(signal_address)) across the entire batch — includinghandle_decrypted_plaintext, which does protobuf decode, SKDM / app-state-key / LID-migration / PDO / history-sync handling, and the app dispatch. None of that touches the Signal session ratchet — onlymessage_decryptdoes — so the lock only needs to cover the crypto.Change
DeferredPlaintextduring the locked decrypt loop, release the guard, then runhandle_decrypted_plaintextover the drained buffer, unlocked.MigrationDecryptResultenum, so multi-payload dispatch order stays uniform. This also collapses four verbose outcome-merge call sites into amatch.A concurrent stanza from the same sender device — e.g. the same participant across two group chats, or the detached LID-migration task, both of which contend on
session_lock_for(signal_address)— can now start decrypting while this one dispatches.Motivation
This matches whatsmeow, which holds the per-sender session lock only around the libsignal decrypt call and handles the decrypted message unlocked. Holding it across dispatch serializes same-sender cross-chat work behind unrelated downstream I/O.
Correctness
handlers/message.rs), which processes one stanza at a time through full dispatch — not by this guard. Releasing the guard earlier cannot reorder delivery.skmsgdecrypt reads that sender key.message_decrypt(the only session-ratchet mutation) still runs under the guard;handle_decrypted_plaintextonly touches sender-key / app-state stores + dispatch, and is already invoked lock-free on the group, newsletter, and PDO paths today.decrypted/duplicate/dispatched/skdm_only/plaintext_failed/had_failure) are all finalized before the function returns, so the PASS 2 gate and the ack fallbacks are unaffected.Testing
cargo test -p whatsapp-rust --lib— 933 passed, 0 failed (incl. themessage::testsdecrypt-path suite), deterministic across runs.cargo clippy -p whatsapp-rust --tests— clean.Notes
permit=N, no cross-sender lock), so this change only marginally widens the racing set, and on the session path it touches that is almost entirely SKDM-only decrypts (no durable row, acked by design) or already-serialized 1:1 chats (chat == sender). A proper fix — a live-path row-before-flush barrier, like the offline drain's commit-batcher — is a separate follow-up.Generated by Claude Code