fix(retry): bound outbound resend rate per group to prevent AccountLocked - #871
Conversation
…cked A bot running this lib hit a 403 AccountLocked. Root cause from production logs: a sustained high rate of outbound retry resends to a single group. During a mass PN->LID migration, hundreds of distinct devices each fail to decrypt the same messages and each send a retry, and we pairwise-resend to every one. The penalized signal is the aggregate per-chat resend rate, not any single device's depth, so MAX_RETRY_COUNT (which trusts the peer-echoed count, stuck at 1) never fires and a per-(chat,msg,requester) cap cuts under 0.5% of the storm. Add a per-group token-bucket rate limiter on the resends we perform, keyed by chat so it bounds the aggregate rate regardless of how many devices drive it. It drops rather than queues when over budget: the requester was already marked for fresh SKDM, so it recovers on the next send and re-requests on its own timer. Refill is lazy off the monotonic clock (no timers, correct over long sessions), the bucket cache is capacity-only (bounded memory), and the same-chat read-modify-write is race-free via single-flight get_with plus a per-bucket mutex. Group-only because DMs have no SKDM fallback. Defaults are conservative (burst 20, refill 10/min) and tunable live via Client::set_resend_rate_limit and BotBuilder::with_resend_rate_limit; resends_throttled_total surfaces storm chats. Builds on the investigation in #870.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a per-chat outbound resend rate limiter using a token-bucket algorithm. A new ChangesPer-chat outbound resend rate limiter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff6b2ad14
ℹ️ 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".
There was a problem hiding this comment.
1 issue found across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
A chronically undecryptable sender (a broken Signal session the peer never re-establishes, or a draining offline queue of messages that can no longer decrypt) fails every message, and we send a retry receipt for each one. The per-message cap (MAX_DECRYPT_RETRIES) bounds receipts per message id, not per sender, so a sender flooding distinct ids is never gated and the aggregate climbs into AccountLocked -- the same anti-abuse class the per-chat resend limiter (oxidezap#871) bounds for outbound resends. Observed in the wild: ~25k retry receipts from ~15 senders over two days. Add a per-sender token-bucket limiter mirroring resend_rate_limiter, keyed by sender. When a sender exhausts its budget, ack-and-drop the stanza (drain the offline queue so the server stops redelivering) instead of asking for a resend forever. The session still self-heals: the bucket refills lazily and any fresh handshake recovers it, so a transient desync (a handful of retries under the burst) is never penalized. The UndecryptableMessage event still fires, so app-side recovery (local session reset) is unaffected. Wired through the public Client API (set_retry_receipt_rate_limit / retry_receipts_throttled_total) and memory diagnostics, mirroring the resend limiter. TokenBucket is shared with resend_rate_limiter. Defaults: burst 10, refill 2/min, capacity 4096 senders.
A chronically undecryptable sender (a broken Signal session the peer never re-establishes, or a draining offline queue of messages that can no longer decrypt) fails every message, and we send a retry receipt for each one. The per-message cap (MAX_DECRYPT_RETRIES) bounds receipts per message id, not per sender, so a sender flooding distinct ids is never gated and the aggregate climbs into AccountLocked -- the same anti-abuse class the per-chat resend limiter (oxidezap#871) bounds for outbound resends. Observed in the wild: ~25k retry receipts from ~15 senders over two days. Add a per-sender token-bucket limiter mirroring resend_rate_limiter, keyed by sender. When a sender exhausts its budget, ack-and-drop the stanza (drain the offline queue so the server stops redelivering) instead of asking for a resend forever. The session still self-heals: the bucket refills lazily and any fresh handshake recovers it, so a transient desync (a handful of retries under the burst) is never penalized. The UndecryptableMessage event still fires, so app-side recovery (local session reset) is unaffected. Wired through the public Client API (set_retry_receipt_rate_limit / retry_receipts_throttled_total) and memory diagnostics, mirroring the resend limiter. TokenBucket is shared with resend_rate_limiter. Defaults: burst 10, refill 2/min, capacity 4096 senders.
Problem
A bot running this lib hit
<failure reason="403" location="vll"/>(AccountLocked). Root cause from production logs: a sustained high rate of outbound retry resends to a single group. During a mass PN->LID migration, hundreds of distinct devices each fail to decrypt the same messages and each send a retry receipt, and we pairwise-resend to every one. The signal WhatsApp anti-abuse penalizes is the aggregate per-chat resend rate (many distinct devices), not any single device's depth. The existingMAX_RETRY_COUNT=5trusts the peer-echoedcount, which stays at 1, so it never fires.This builds on the investigation in #870 by @Salientekill (thanks for the logs and the diagnosis). Measured against the real logs, a per-(chat,msg,requester) cap cuts under 0.5% of the storm, because the storm is a cross-device fan-out rather than a single device looping, so a per-device key never accumulates. The fix has to aggregate across devices.
Fix
A per-group token-bucket rate limiter on the resends we actually perform.
get_withplus a per-bucket mutex.MAX_RETRY_COUNT) rather than risk a dropped delivery.Client::set_resend_rate_limitandBotBuilder::with_resend_rate_limit.resends_throttled_total()surfaces storm chats.Not WA Web behavior (WA Web caps only on the peer-echoed count plus delivery eligibility); added as a deliberate safety valve for the production 403, since the fan-out is something WA Web itself does not defend against.
Tests
resend_rate_limiter): under/over burst, refill over time, burst cap, disabled, per-chat isolation, live retune clamp, and a multi-threaded no-bypass-under-contention test.retry): the limiter is reachable and tunable through the publicClientAPI, andhandle_retry_receiptdrops a throttled group resend (returnsOk, sends nothing) while keeping the message cached and clearing the in-progress marker.Verify
cargo fmt --all,cargo clippy --all-targets -- -D warningsclean.cargo test -p whatsapp-rust --lib(826 tests) pass.