Review of upstream PR #983 (oxidezap/whatsapp-rust)

[marchugon]

FriendlyReviewer review of oxidezap/whatsapp-rust#983.

---

Original PR description:

Summary

Second of the recv post-decrypt dispatch series (follow-up to oxidezap#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_batch held the per-sender ratchet guard (session_lock_for(signal_address)) across the entire batch — including handle_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 — only message_decrypt does — so the lock only needs to cover the crypto.

Change

• Buffer each decrypted plaintext into a DeferredPlaintext during the locked decrypt loop, release the guard, then run handle_decrypted_plaintext over the drained buffer, unlocked.
• The rare PN→LID migration retry helper buffers its plaintext into the same queue and returns a small MigrationDecryptResult enum, so multi-payload dispatch order stays uniform. This also collapses four verbose outcome-merge call sites into a match.

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

• Delivery order is unchanged. Per-chat in-order delivery is owned by the serial chat-lane worker (handlers/message.rs), which processes one stanza at a time through full dispatch — not by this guard. Releasing the guard earlier cannot reorder delivery.
• SKDM still precedes group decrypt. The buffer drains before the function returns, so a pkmsg's sender-key distribution is applied before PASS 2's skmsg decrypt reads that sender key.
• Ratchet stays serialized. message_decrypt (the only session-ratchet mutation) still runs under the guard; handle_decrypted_plaintext only touches sender-key / app-state stores + dispatch, and is already invoked lock-free on the group, newsletter, and PDO paths today.
• Outcome flags (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. the message::tests decrypt-path suite), deterministic across runs.
• cargo clippy -p whatsapp-rust --tests — clean.
• CI green: E2E Tests, wasm32 release, Build & Test, Build & Lint (all features), Test Stable (no-simd), Clippy, Format, Binary Size (−1.5 KiB), and the CodSpeed benchmarks. No new dependencies.

Notes

• A reviewer raised a durability concern: dropping the guard lets a same-sender/different-chat stanza reach the stanza-end whole-cache flush before this message commits its pending-inbound row, so a crash in that window could persist the ratchet advance without a durable row. This is a pre-existing property, not introduced here — the live-path flush already persists any uncommitted stanza's ratchet via cross-sender concurrency (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

[friendlyreviewer-staging]

Hi there 👋

⛅ Tech
3 medium, 1 low

The Merge Request introduces two key architectural improvements: lock narrowing in handle_decrypted_plaintext to release the per-sender guard before plaintext dispatch, and extraction of MigrationDecryptResult to simplify merge-site semantics. Both changes are logically sound and backward-compatible. However, critical test-coverage gaps remain: (1) no concurrency test validates that same-sender stanzas can interleave during dispatch under the narrow lock; (2) no test verifies FIFO ordering of deferred buffer for multi-payload session batches. Additionally, a minor fragility in variable shadowing and a type-unsafe benchmark helper should be addressed for long-term maintainability.

---

Worth checking

• 🟡 src/message/receive.rs (L1126)
  Missing concurrency test: No functional test validates that concurrent same-sender stanzas can start decryption while one dispatches, potentially deadlocking or failing under the narrowed lock. The existing benchmark is single-threaded and does not cover this scenario.
• 🟡 src/message/receive.rs (L1139)
  Missing ordering test: No test verifies that deferred buffer preserves FIFO ordering for multi-payload session batches (e.g., pkmsg followed by msg). Wire-order correctness is not explicitly validated.
• 🟡 src/message/receive.rs (L931)
  Variable shadowing fragility: The local enc_type is shadowed from EncType to &'static str. While currently correct because as_wire_str() returns &'static str, a future refactor could silently break this code. Use a distinct variable name (e.g., enc_type_str) to decouple from the static lifetime guarantee.

Small things (take or leave)

• 🔵 src/message/tests.rs (L10277)
  Benchmark helper type-unsafe: The bench_feed helper takes enc_type: &'static str and round-trips through string → Enum → string. An invalid type string would cause an unwrap() panic. Prefer using EncPayload::from_parts with an EncType variant to avoid the string round-trip and improve type safety.

Review time: 3m 4s