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

[marchugon]

FriendlyReviewer review of oxidezap/whatsapp-rust#985.

---

Original PR description:

Summary

A WA-Web-compliant, decoupled alternative to the inbound retry-receipt quarantine in oxidezap#982 (thanks @Salientekill for the diagnosis and production data that made this possible), plus the compliant efficiency win that helps every send by default.

Two things:

1. An opt-in RetryAdmission hook so a quarantine can live in operator code, keeping the SDK's default path byte-for-byte WA Web.
2. markForgetSenderKey now updates the sender-key-device cache in place, per device, matching WA Web, instead of invalidating the whole group. A generation stamp keeps the warm-send memo correct without any cross-cache invalidation.

Why not land oxidezap#982 as-is

Cross-checked against the captured WA Web bundle:

• WA Web has no volume-based throttle on inbound retry receipts. WAWebHandleRetryRequest serializes per chat (onMessageQueue + sendMsgQueueMap), refuses past MAX_RETRY, and otherwise processes every receipt. Its only gates are semantic (getMsgIfAuthorized → isRetryEligible: ALREADY_DELIVERED, CHANGED_IDENTITY, RECORD_MISSING, DEVICE_NOT_RECIPIENT, HIGH_RETRY_COUNT, MESSAGE_EXPIRED), never "this member is asking too often."
• The send side is faithful too: GroupSkmsgJob marks markHasSenderKey(x, M) over the whole skDistribList (failed devices included). So the "broken member retries every message" loop is WA Web's own design, and the per-receipt session rebuild is work WA Web does too. WA Web survives it only because a single-user client never sends at bot volume.

A per-(chat, requester) quarantine that drops eligible repair requests is a deliberate divergence, fine for a bot operator but not a default the library should impose.

1. RetryAdmission hook (opt-in, zero overhead when unused)

• Trait (src/types/retry_admission.rs) mirroring the existing InboundDurabilityHook idiom (OnceLock, builder-less Client::set_retry_admission). Object-safe, WASM-safe.
• Synchronous admit(chat, requester, retry_count) -> bool: a gate never needs to wait, and a sync verdict cannot stall retry processing while the pending_retries scopeguard is held.
• Gate in handle_retry_receipt, placed exactly where feat(retry): quarantine per-(chat, requester) inbound retry receipts oxidezap/whatsapp-rust#982 put its gate (after is_peer, before the group-info fetch, the unknown-device rotateKey block, markForgetSenderKey, key-bundle processing, and the resend). Own companion devices (is_peer) and all DMs never reach the policy.
• Zero overhead when unused: with no policy registered the check is a single OnceLock::get().
• Log demotion: the per-receipt Marked … for fresh SKDM line drops from info to debug (a broken cohort emits tens of thousands/day; WA Web logs it at its verbose level).
• examples/retry_quarantine.rs — a token-bucket quarantine keyed by (chat, requester), burst 2 / refill 2/day, built on the hook. This is @Salientekill's mechanism from feat(retry): quarantine per-(chat, requester) inbound retry receipts oxidezap/whatsapp-rust#982, now operator-owned and tunable.

2. In-place sender-key-device cache forget (compliant perf win, default)

WA Web's markForgetSenderKey flips one device's flag in the participant record in place. Ours invalidated the whole-group sender_key_device_cache, forcing the next send to re-read and re-parse every device row from the DB (a 1000+ device map re-materialized per send during a storm).

• SenderKeyDeviceMap stores has_key as an AtomicBool and carries an AtomicU64 generation. SenderKeyDeviceCache::mark_forgotten flips only the touched devices cold in a live cache entry (no invalidation, no re-read), and bumps the generation once — only when a flag actually transitions warm→cold (swap(false)), so a retry storm of duplicate cold marks never churns it.
• The warm-marking path still invalidates the group, since marking a device warm can introduce (user, device) pairs the cached map doesn't have yet.
• The warm-send memo stays correct without a second invalidation. skdm_warm_memo lets a warm repeat send skip the O(devices) filter_skdm_targets scan; it now stores the map's generation alongside the two Weak Arcs (Weak<ResolvedGroupDevices>, Weak<SenderKeyDeviceMap>, u64). An in-place cold flip keeps the same Arc, so pointer identity alone can't see it — the generation can. The send path loads the generation before filtering (Acquire), so a flip racing in after that stamps the memo as already stale and the next send re-runs the filter. A single Acquire/Release generation pair carries the freshness (the same contract the device-registry generation gives membership), so there is no separate memo invalidation and therefore no cross-cache ordering window.
• The DB write stays the source of truth; a cache miss rebuilds from it.

What stays out of the core (vs oxidezap#982)

retry_mark_quarantine_capacity, the StatsSnapshot and MemoryReport fields, the lifecycle wiring, and any on-by-default behavior.

Remaining follow-up (separate PR)

Close the isRetryEligible parity gap — WA Web tracks per-(message, recipient-device) delivery state and rejects ALREADY_DELIVERED / CHANGED_IDENTITY. We don't, so we can't. handle_retry_receipt now documents which reject reasons we already enforce (HIGH_RETRY_COUNT, MESSAGE_EXPIRED / RECORD_MISSING, DEVICE_NOT_IN_DATABASE) and which stay a known gap. Closing the rest needs a per-recipient receipt store (schema + hot-path writes per recipient), which is a substantial standalone feature and out of scope here.

Tests

Both the happy and the failure/edge paths are covered:

• Cache generation — in-place flip advances the generation while keeping the same Arc; a duplicate-cold or absent-device mark must not advance it; a mixed batch flips every present device and bumps the generation exactly once; a cache miss is a no-op.
• WA Web warm gate — a device is warm only when it and its primary (device 0) both hold the key; a cold or absent primary makes the whole user cold.
• Malformed input — from_db_rows skips corrupt / partially-migrated JID rows without dropping the valid devices around them.
• Own-device exclusion — a cold mark that names our own device plus a broken member flips only the member (WA Web !isMeDevice), and is a full no-op (no DB write, no generation bump) when every named device is our own.
• Quarantine example — per-pair bounding and isolation, burst = 0 disable, fail-open past capacity, gradual token refill (recovery, not a reset), and the idle clamp that stops an idle pair banking an unbounded reserve.

cargo fmt --all --check clean; cargo clippy -p whatsapp-rust --tests --examples clean; 942 lib tests pass; examples/retry_quarantine.rs builds and its 5 unit tests pass.

@Salientekill — would you try this in your 1000-member group? Register RetryQuarantine from the example via client.set_retry_admission(...) with burst 2 / refill 2/day and confirm it bounds the storm the way your version did.

[friendlyreviewer-staging]

Hi there 👋

🌥️ Tech
1 high, 2 medium, 2 low

The merge request introduces a RetryAdmission trait and its integration into the retry handling pipeline, along with sender-key device cache improvements. Overall, the design is solid: the trait is object-safe, the OnceLock-based dispatch is zero-overhead when unset, the gate placement in handle_retry_receipt is correct, and the public API re-exports are complete. The example implementation of a token-bucket quarantine is well-tested. However, a substantive race window was identified in the warm-memo generation stamp for SKDM filtering: the cached generation is loaded before an async yield, allowing a concurrent mark_forgotten to be missed until the next send. Additionally, minor issues include a redundant cache invalidation, missing edge-case test, a suboptimal clone, and a documentation gap for relaxed loads in the non-memoized path. The log demotion from info to debug is correctly applied. Addressing the race window is recommended to align documentation with implementation.

---

These might need a close look

• 🔴 src/send/mod.rs (L1097)
  Race window in warm-memo generation stamp: cached_map_gen is loaded before an async call (resolve_group_devices_memoized), so a concurrent mark_forgotten during that gap is not detected. The memo check may incorrectly skip filter_skdm_targets, causing one missed SKDM per racing mark. Update the documentation to accurately describe the best-effort stamp or reload the generation after the async gap.

Worth checking

• 🟡 src/send/mod.rs (L1138)
  Double invalidation of sender_key_device_cache: update_sender_key_devices calls set_sender_key_status_for_devices which already invalidates the cache, then issues the same invalidation again. Remove the redundant invalidation or restructure to avoid the extra async operation.
• 🟡 src/sender_key_device_cache.rs (L94)
  Documentation gap for relaxed loads in device_and_primary_warm: the function uses Ordering::Relaxed which is correct when preceded by an Acquire load of generation(), but the non-memoized path in filter_skdm_targets does not perform such an acquire. Document the caller responsibility or acknowledge the relaxed consistency.

Small things (take or leave)

• 🔵 src/sender_key_device_cache.rs (L192)
  No test coverage for empty devices slice in mark_forgotten. Adding a test for mark_forgotten(group, &[]) would document that the function correctly skips the loop and does not bump the generation.
• 🔵 src/client/sender_keys.rs (L75)
  Redundant Vec<Jid> clone in the cold-mark branch: kept could be collected as Vec<Jid> directly from the filter to avoid an extra allocation and clone. Minor optimization opportunity.

Review time: 4m 4s