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

[marchugon]

FriendlyReviewer review of oxidezap/whatsapp-rust#982.

---

Original PR description:

Problem

In large groups, a cohort of members whose pairwise sessions never establish (dead registrations, exhausted prekeys, re-registered LIDs) sends a retry receipt for every single group message. Each receipt pays markForgetSenderKey (a DB write + a whole-group sender-key cache invalidation), bundle processing and possibly a resend — all upstream of the per-chat resend cap (oxidezap#871), which only bounds the resend itself.

Production evidence (bot in a 1012-participant group, 3.5 days of debug logs):

• 58,554 Marked <jid> for fresh SKDM ... due to retry receipt lines — 99.7% from this one group, coming from 468 distinct members (~68 marks/member/day ≈ one per bot message: they fail on every message, i.e. their sessions never repair).
• The per-chat cap blocked ~75% of the resends (23k+19k+1k throttled) — but none of the mark/bundle work, which runs before the cap check in handle_retry_receipt.
• Related root cause on the send side: after the server ACK the whole SKDM target set is marked has_key=true, including devices whose distribution failed (406 / no bundle) — wacore/src/send/group.rs — relying on the retry-receipt path to repair, which produces a permanent warm/cold oscillation for this cohort. (A follow-up PR could add a per-device re-target cooldown; this PR bounds the receipt side.)

Mechanism

RetryMarkQuarantine: a token bucket keyed by (chat user, requester user) — burst 2, refill 2/day. One mark is enough to repair a healthy member (the next send carries the SKDM), so past the burst, further receipts from the same pair are dropped before any repair work; the refill keeps genuine recovery possible. Device is excluded from the key on purpose: WA Web re-targets the whole user when the primary goes cold, so all devices of a broken account share one budget.

With the observed numbers this cuts the repair-path work from ~16.7k/day to ~470×3/day worst case (~97%), while leaving first-time repairs untouched.

Implementation

• Reuses the existing TokenBucket + capacity-only Cache machinery from the resend limiter (lazy monotonic refill, no timers, allocation only on miss).
• Gate placed in handle_retry_receipt right before update_local_signal_session, after the cheap early-outs and the concurrency guard; early return releases the pending-retry scopeguard normally.
• burst = 0 disables; live-tunable via Client::set_retry_mark_quarantine(burst, refill_per_day).
• Observability: StatsSnapshot.retry_receipts_quarantined + retry_mark_quarantine_pairs in MemoryDiagnostics.
• Unit test covers per-pair bounding, pair isolation and the disable knob.

Relation to oxidezap#924: that PR bounds outbound retry receipts (our own decrypt failures); this one bounds the inbound direction, which is where the group-storm cost actually lands.

[marchugon]

make a full new review

[friendlyreviewer-staging]

Hi there 👋

🌤️ Tech
2 low

The MR introduces a RetryMarkQuarantine rate limiter to throttle retry receipts. The implementation is well-structured, following the existing ResendRateLimiter pattern. The token bucket with lazy monotonic refill and capacity-only eviction is correct. The integration gate in retry.rs is positioned optimally to avoid unnecessary work. Client-side wiring in accessors.rs, lifecycle.rs, and CacheConfig is consistent and complete. Observability via StatsSnapshot is added. Minor documentation/style observations were noted but do not affect correctness or performance.

---

Small things (take or leave)

• 🔵 src/cache_config.rs (L1)
  The new field retry_mark_quarantine_capacity is properly documented, added to Debug and Default. Ensure external struct-literal constructions use ..CacheConfig::default() to avoid breakage (already the case).
• 🔵 src/client/accessors.rs (L1)
  Public accessor set_retry_mark_quarantine and wiring to StatsSnapshot and MemoryReport are correctly implemented following the existing pattern.

Review time: 4m 20s