fix(offline): clear self-fanout with sender receipt, not a bare ack - #659
Merged
Conversation
A self-fanout (our own outgoing message echoed back to this companion device: is_from_me + a `recipient`) was acknowledged with a bare `<ack class="message">` transport ack. WhatsApp Web (`isMeAccount(author) => RECEIPT_TYPE.SENDER`) and whatsmeow (`IsFromMe => "sender"`) clear it with a `<receipt type="sender" recipient=...>` instead. The server's offline queue never releases the stanza on the bare ack, so it replays the message every reconnect and a ~50min server-side GC force-closes the stream with `<stream:error><ack class="message" type="text" id=.../></stream:error>` + `<xmlstreamend/>`. This is acute for bot self-fanouts (recipient is a `@bot`), which never decrypt locally, so they loop forever until the connection resets. `should_send_delivery_receipt` now allows is_from_me DMs that carry a `recipient` (user or bot, not group/status/newsletter), and `build_delivery_receipt_node` emits `type="sender"` plus the device-stripped `recipient`, keeping `to` device-preserving. PR #647/#648 made these messages acked at all (previously dropped) but with the wrong stanza kind; this sends the kind the server actually accepts. A recipient-less own message (self-note) stays skipped. encode_ack_bytes is untouched (recipient on the transport ack stays, per #633).
📝 WalkthroughSummary by CodeRabbit
WalkthroughOwn outgoing self-fanout messages (from-me + recipient, non-group/status/newsletter) are now eligible for WA‑Web-style delivery receipts using type="sender" and a device-stripped recipient; client and message-layer paths updated and tests adjusted to emit those receipts and avoid transport retry loops. ChangesSelf-fanout receipt
Sequence Diagram(s)sequenceDiagram
participant Handler as handle_decrypt_failure
participant Client
participant Builder as build_delivery_receipt_node
participant Wire as Wire Output
Handler->>Handler: detect info.source.is_self_fanout()
Handler->>Client: request send_delivery_receipt(type="sender")
Client->>Builder: build node (type="sender", recipient=device-stripped)
Builder-->>Wire: emit <receipt type="sender" recipient=... to=...>
Handler-->>Handler: return early (no retry / no bare ack)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Alright — this needs to work correctly. Verify the early-return on decrypt failure and the test assertions that no bare transport or retry receipts are emitted. 🚥 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 |
jlucaso1
marked this pull request as ready for review
May 29, 2026 13:17
![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: 5b78e83c5f
ℹ️ 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".
Benchmark Results1 regression(s) detected (>2% threshold):
66 unchanged benchmark(s)
|
![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/receipt.rs`:
- Around line 50-61: In build_delivery_receipt_node ensure a receipt never
carries both type="peer_msg" and a recipient: when adding the recipient
attribute (currently guarded by is_self_fanout and info.source.recipient) also
guard out MessageCategory::Peer (i.e. require is_self_fanout && info.category !=
MessageCategory::Peer && let Some(recipient) = &info.source.recipient) before
calling builder.attr("recipient", ...); this will prevent a peer-category
receipt from including the recipient attribute while leaving the existing
type-selection logic (info.category, is_self_fanout, active, is_status)
unchanged.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac8ca082-2415-4e7c-aa20-1aed2563f761
📒 Files selected for processing (3)
src/message.rssrc/receipt.rswacore/src/stanza/receipt.rs
Address review feedback: - P1 (decrypt-failure path): handle_decrypt_failure sent a bare transport ack for self-fanouts, which the server ignores, so an already-stuck account (whose session counter has advanced past the duplicate state into BadMac) kept looping. Route self-fanouts through the sender receipt there too, skipping the futile retry-to-self. - peer + recipient: a peer-synced message that also looks like a self-fanout must keep type="peer_msg" and carry NO recipient (WA Web `!l` guard). build_delivery_receipt_node now gates the sender shape on `category != Peer`. - is_group robustness: extract MessageSource::is_self_fanout(), which also excludes chat.is_group() (the own-from parser derives chat from recipient and leaves is_group defaulted), shared by both should_send_delivery_receipt copies and the decrypt-failure path. - magic strings: add ReceiptType::as_wire_str() and use it for the sender/peer_msg/inactive type attrs instead of literals. - stale comment on ack_received_message, misleading debug log label, and the wacore copy's "keep in sync" note (status handling intentionally differs) all corrected. Adds tests: self_fanout_decrypt_failure_acked_via_sender_receipt, peer_self_fanout_is_peer_msg_without_recipient, self_fanout_is_sender_even_when_inactive.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/stanza/receipt.rs (1)
215-231: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd the negative parity cases here too.
This mirror now shares the self-fanout rule with
Client::should_send_delivery_receipt, but the new coverage only proves the positive DM path. Add own status/group cases with arecipientso this copy can’t drift silently from the authoritative gate.🤖 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 `@wacore/src/stanza/receipt.rs` around lines 215 - 231, The test allow_self_fanout_with_recipient currently only asserts the positive direct-message path; add negative-parity cases mirroring Client::should_send_delivery_receipt to ensure parity: create additional MessageInfo/MessageSource instances (using MessageInfo, MessageSource, setting is_from_me = true and recipient = Some(...)) where chat represents status and group JIDs and assert should_send_delivery_receipt returns false for those, and also add the corresponding positive/negative permutations as needed so this test suite covers own status/group with a recipient and cannot drift from the authoritative gate.
🤖 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.rs`:
- Around line 586-593: The current branch unconditionally treats any
decrypt-failure self-fanout as a delivery-receipt candidate by checking
info.source.is_self_fanout() and calling client.send_delivery_receipt(&info);
tighten this to match protocol by also checking the same predicate used on the
normal ack path (should_send_delivery_receipt or equivalent) so you only send a
receipt when both info.source.is_self_fanout() and
should_send_delivery_receipt(&info) are true; update the if condition
accordingly so client.send_delivery_receipt(&info).await is only invoked for
self-fanouts that are actually eligible for delivery receipts.
In `@src/receipt.rs`:
- Around line 301-309: The debug log always labels the non-peer/non-sender
branch as Delivered even though build_delivery_receipt_node() emits
type="inactive" for passive companions; update the receipt_kind selection in the
block that currently uses info.category, MessageCategory::Peer,
info.source.is_self_fanout(), ReceiptType::PeerMsg, ReceiptType::Sender and
ReceiptType::Delivered to detect the same "companion is passive/inactive"
condition used by build_delivery_receipt_node() and return a new
ReceiptType::Inactive (or the existing enum variant used for inactive receipts)
instead of Delivered so the debug line continues to call
receipt_kind.as_wire_str() and accurately logs "inactive" when appropriate.
---
Outside diff comments:
In `@wacore/src/stanza/receipt.rs`:
- Around line 215-231: The test allow_self_fanout_with_recipient currently only
asserts the positive direct-message path; add negative-parity cases mirroring
Client::should_send_delivery_receipt to ensure parity: create additional
MessageInfo/MessageSource instances (using MessageInfo, MessageSource, setting
is_from_me = true and recipient = Some(...)) where chat represents status and
group JIDs and assert should_send_delivery_receipt returns false for those, and
also add the corresponding positive/negative permutations as needed so this test
suite covers own status/group with a recipient and cannot drift from the
authoritative gate.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c086be36-002f-4059-a96b-d9b710d8ea04
📒 Files selected for processing (5)
src/message.rssrc/receipt.rswacore/src/stanza/receipt.rswacore/src/types/message.rswacore/src/types/presence.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0495e61d4
ℹ️ 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".
…routing Apply the still-valid review nits and lock the verified decisions with tests: - send_delivery_receipt debug log mirrored build_delivery_receipt_node's type selection, so a passive companion now logs "inactive" instead of "delivery". - wacore should_send_delivery_receipt test gains negative parity (own status / group with a recipient => false), matching the client-crate gate so the two copies can't drift. - new test own_bot_author_dm_acks_not_sender_receipt: when WE are the bot author (own DM, sender on @bot, to a user) WA Web emits a bot-invoke-response <ack> (MsgSendReceipt `!chat.isBot() && author.isBot()`), NOT a sender receipt; locks the ack_received_message ordering against a regression. Deliberately NOT changed (verified against captured-js + the production log): - decrypt-failure path keeps the immediate sender receipt (no retry-then-clear): the stuck stanza is redelivered once per ~50min connection, so retrying to the cap would prolong the disconnect loop ~4h; clearing on the first cycle is correct and a sender receipt for our own message is semantically valid. - bot-author branch order is kept (Codex P2 would route it to a sender receipt, diverging from WA Web's bot-invoke-response ack).
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.rs`:
- Around line 7798-7813: The negative assertion that OWN BOT1 doesn't receive a
<receipt> is currently checked immediately after detecting the <ack>, which can
miss a receipt sent a tick later; after locating the ack via
find_message_ack(&transport.sent()) (the loop around tokio::time::sleep), add a
short deterministic settle window (e.g., poll for ~100–200ms or a few iterations
with small sleeps) examining transport.sent() for any find_receipt(...,
"OWNBOT1") and fail if any receipt is observed; in short, once found is Some,
continue polling for a short duration and assert that
find_receipt(&transport.sent(), "OWNBOT1") remains None for the whole settle
period (and fail if it ever appears).
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11a83683-07e6-4189-9aff-137fccf2ffe1
📒 Files selected for processing (3)
src/message.rssrc/receipt.rswacore/src/stanza/receipt.rs
…fail gate Address the remaining review nits (all verified; no code blocker found): - handle_decrypt_failure self-fanout short-circuit now gates on `is_self_fanout() && should_send_delivery_receipt()`, matching the normal ack path's eligibility (coderabbit). is_self_fanout() stays the load-bearing prefix; the extra check only affects the unreachable empty-id case. - own_bot_author_dm_acks_not_sender_receipt: added a short settle window so a future regression that emitted a receipt on a later tick can't slip past the negative assertion. (No current race: ack_received_message is synchronous and the bot-author branch returns before the receipt branch.) - bot_self_fanout test now uses a device-bearing sender and asserts the device survives into the receipt `to` (end-to-end guard for #649, previously only covered by the isolated builder unit test). - is_self_fanout_matches_only_own_dm_with_recipient: isolated coverage for each exclusion, including the load-bearing chat.is_group() guard with is_group=false (the own-from parser path). - as_wire_str_round_trips_through_parse: guards ReceiptType::as_wire_str (the hand-maintained inverse of parse) against hyphen/underscore drift.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2242de2c1
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/types/presence.rs (1)
90-108: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCollapse
From<String>intoparseso these tables can't drift.Look, we now ship three hand-maintained copies of the same string↔variant mapping:
parse(51-67),From<String>(90-108), and the newas_wire_str(71-87). Your new round-trip test guardsas_wire_stragainstparse, which is great — but nothing guardsFrom<String>againstparse. If someone adds a variant toparseand forgetsFrom, they silently diverge and the test stays green. That's exactly the kind of thing that needs to just work. HaveFromdelegate to the single source of truth.♻️ Delegate to a single mapping
impl From<String> for ReceiptType { fn from(s: String) -> Self { - match s.as_str() { - "" | "delivery" => Self::Delivered, - "sender" => Self::Sender, - "retry" => Self::Retry, - "enc_rekey_retry" => Self::EncRekeyRetry, - "read" => Self::Read, - "read-self" => Self::ReadSelf, - "played" => Self::Played, - "played-self" => Self::PlayedSelf, - "server-error" => Self::ServerError, - "inactive" => Self::Inactive, - "peer_msg" => Self::PeerMsg, - "hist_sync" => Self::HistorySync, - _ => Self::Other(s), - } + Self::parse(&s) } }Note:
parse's fallback doesother.to_string(), so the movedsbecomes a clone of itself — a negligible allocation on the coldOtherpath. If you'd rather not pay even that, keep the explicit_ => Self::Other(s)arm but route the rest through a shared helper.🤖 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 `@wacore/src/types/presence.rs` around lines 90 - 108, The impl From<String> for ReceiptType duplicates the string->variant mapping; change it to delegate to the central parser to avoid drift by implementing From<String> { fn from(s: String) -> Self { ReceiptType::parse(s.as_str()) } } (or otherwise call the existing parse(&str) helper), so the mapping lives only in ReceiptType::parse (which is the single source of truth alongside as_wire_str).
🤖 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 `@wacore/src/types/presence.rs`:
- Around line 90-108: The impl From<String> for ReceiptType duplicates the
string->variant mapping; change it to delegate to the central parser to avoid
drift by implementing From<String> { fn from(s: String) -> Self {
ReceiptType::parse(s.as_str()) } } (or otherwise call the existing parse(&str)
helper), so the mapping lives only in ReceiptType::parse (which is the single
source of truth alongside as_wire_str).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32db9193-52b1-4837-86ed-2c4c18133e24
📒 Files selected for processing (3)
src/message.rswacore/src/types/message.rswacore/src/types/presence.rs
From<String> duplicated the string->variant match already in parse(), so the mapping lived in three places (parse, From<String>, as_wire_str) and could drift. Extract the known-variant mapping into a private from_known(&str) -> Option<Self>; parse() clones for the Other fallback, From<String> moves the owned String for it (no extra allocation). No behavior change.
…pt failure Codex P2 (valid): the decrypt-failure self-fanout branch fired for a bot-authored own DM to a user (sender on @bot, non-bot chat), sending a type="sender" receipt — but the success/duplicate path (ack_received_message) routes that same shape to the bot-invoke-response bare <ack> first (WA Web `!chat.isBot() && author.isBot()` => sendBotInvokeResponseAcks), locked by own_bot_author_dm_acks_not_sender_receipt. The two paths diverged for an undecryptable bot-authored own DM. Extract the predicate to MessageSource::is_bot_authored_non_bot_chat() (DRY, shared by ack_received_message and handle_decrypt_failure) and exclude it from the decrypt-failure sender-receipt short-circuit, so a bot-authored own DM stays on the bare-ack path on both the success/duplicate and failure paths. Adds bot_author_self_fanout_decrypt_failure_not_sender_receipt.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.rs`:
- Around line 7492-7501: The test only asserts that a sender <receipt> never
appears but doesn't assert that the positive bot-invoke ack path occurred;
before the existing negative settle loop (the for _ in 0..5 loop that calls
find_receipt(&transport.sent(), "OWNBOTFAIL1")), add a positive assertion that
the bot-invoke response ack was emitted — e.g., use
find_receipt(&transport.sent(), "OWNBOTFAIL1") (or the helper that inspects ack
class "message") and assert it is Some and/or has class "message" immediately
prior to the settle loop; keep the existing negative assertions afterwards to
ensure the sender receipt remains absent.
In `@wacore/src/types/message.rs`:
- Around line 81-83: The predicate in is_bot_authored_non_bot_chat mixes raw
Server::Bot comparison with self.sender.is_bot(); either replace the chat-side
check with the symmetric !self.chat.is_bot() (mirroring self.sender.is_bot())
or, if JidExt::is_bot() contains different logic, update the function to use the
correct equivalent check for chat (and remove the Server import) after verifying
JidExt::is_bot() implementation; locate the is_bot_authored_non_bot_chat method
and JidExt::is_bot() to confirm equivalence and apply the appropriate change.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8be21d9-d1a9-4b68-a40f-cabb9101565f
📒 Files selected for processing (3)
src/message.rswacore/src/types/message.rswacore/src/types/presence.rs
Two valid review nits: - is_bot_authored_non_bot_chat used `chat.server != Server::Bot` while the sender side used `sender.is_bot()`. JidExt::is_bot() also matches the legacy PN-form Meta bot (1313555*/131655500*), so the two differed for a legacy Meta bot chat. WA Web uses `!chat.isBot() && author.isBot()` (the doc comment already says so), so switch the chat side to `!self.chat.is_bot()` for symmetry and WA-Web fidelity (a legacy Meta bot 1:1 now takes the receipt path, matching WA Web, instead of the bare bot-invoke ack). Drops the now unused Server import. - bot_author_self_fanout_decrypt_failure_not_sender_receipt only asserted the absence of a sender receipt; add a positive assertion that the message IS cleared via the bare <ack class="message"> so the test can't pass on a no-op.
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)
wacore/src/types/presence.rs (1)
26-96: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSwitch
ReceiptTypeto#[derive(WireEnum)]and remove the hand-rolled wire/serde mapping
Inwacore/src/types/presence.rs, you can’t have two separate maps (from_known/parseandas_wire_str) that can drift—WireEnumis designed to be the single source of truth for unit-string wire values, including#[wire_default]and the catch-all#[wire_fallback]variant (yourOther(String)case). Also drop#[derive(..., Serialize, Deserialize)]+#[serde(from = "String")]; theWireEnumderive already providesSerialize/Deserializedelegating toas_str().🤖 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 `@wacore/src/types/presence.rs` around lines 26 - 96, The ReceiptType enum currently implements manual wire-string mapping via from_known, parse, and as_wire_str and also derives Serialize/Deserialize with serde; replace this by deriving WireEnum on ReceiptType, remove the hand-rolled functions (from_known, parse, as_wire_str) and the serde derives/#[serde(from = "String")], and instead annotate variants to use #[wire_default] for the canonical default (e.g., Delivered if desired) and #[wire_fallback] on Other(String) to capture unknown strings; keep the enum variants (Delivered, Sender, Retry, EncRekeyRetry, Read, ReadSelf, Played, PlayedSelf, ServerError, Inactive, PeerMsg, HistorySync, Other(String)) but delete the from_known/parse/as_wire_str implementations so WireEnum provides Serialize/Deserialize and wire mapping.
🤖 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 `@wacore/src/types/presence.rs`:
- Around line 26-96: The ReceiptType enum currently implements manual
wire-string mapping via from_known, parse, and as_wire_str and also derives
Serialize/Deserialize with serde; replace this by deriving WireEnum on
ReceiptType, remove the hand-rolled functions (from_known, parse, as_wire_str)
and the serde derives/#[serde(from = "String")], and instead annotate variants
to use #[wire_default] for the canonical default (e.g., Delivered if desired)
and #[wire_fallback] on Other(String) to capture unknown strings; keep the enum
variants (Delivered, Sender, Retry, EncRekeyRetry, Read, ReadSelf, Played,
PlayedSelf, ServerError, Inactive, PeerMsg, HistorySync, Other(String)) but
delete the from_known/parse/as_wire_str implementations so WireEnum provides
Serialize/Deserialize and wire mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c1d6e26-0857-4b5f-a8d6-11fb5a058e25
📒 Files selected for processing (3)
src/message.rswacore/src/types/message.rswacore/src/types/presence.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An account in production disconnects every ~50 minutes in a tight loop. The server closes the stream with:
Keepalive ping/pong works fine (~150ms RTT) between the drops, so this is not a connection-health issue — it is a server-side offline-queue GC firing on a message that never gets cleared. The same message (and its sibling) is redelivered on every reconnect (once per ~50min connection:
offline="0|1|2…", 100+ times across 12h).Root cause
The stuck stanza is a self-fanout to a bot: our own outgoing prompt to a
@bot, fanned out by the server back to this companion device. Sofromis our own LID,recipientis the bot, andis_from_me = true.The dispatch path (
ack_received_message) hitshould_send_delivery_receipt, which returnedfalseforis_from_me, and fell through to a bare<ack class="message">transport ack. The decrypt-failure path (handle_decrypt_failure) likewise sent only a retry + bare transport ack.WhatsApp Web (
Send/DeliveryReceiptJob.js:isMeAccount(author) => RECEIPT_TYPE.SENDER) and whatsmeow (sendMessageReceipt:IsFromMe => "sender") instead clear an own message with a<receipt type="sender" recipient=...>. The server's offline queue only releases a self-fanout when it sees that sender receipt; a bare transport<ack>is ignored. The message stays pending, gets replayed every reconnect, and the ~50min GC resets the stream. This is acute for bot self-fanouts, which never decrypt locally (a duplicate on the first cycle, then BadMac once the local Signal counter advances), so they loop forever.Verified against
docs/captured-js/(Handle/MsgSendReceipt.js,Send/DeliveryReceiptJob.js,Send/OfflineDeliveryReceiptJob.js,Send/ReceiptJobCommon.js) and whatsmeow, with an adversarial pass over the production wire log.Why #647/#648 helped but did not fix it
Those PRs made duplicate/undecryptable messages get acked at all (previously they were silently dropped, which also stalled the queue). But they emit the wrong stanza kind for a self-fanout — a bare
<ack>rather than the sender<receipt>the server expects. So the disconnects became less frequent but never stopped.Hypotheses ruled out
from=PNnamespace mismatch — refuted: WA Web (MsgSendAck.js) and whatsmeow both setfrom = own device PN. Log counter-example: self-fanouts with a@lidrecipient clear fine with the same ack shape; only@botrecipients stick.recipientfrom the ack — refuted and dangerous: PR fix(client): preserve recipient in <ack>, soften unknown stream:error #633 addedrecipientbecause dropping it produced this exact<stream:error><ack/>.encode_ack_bytesis left untouched.Fix
MessageSource::is_self_fanout()(shared helper):is_from_me && recipient.is_some() && !is_group && !chat.is_group() && !chat.is_status_broadcast() && !chat.is_newsletter(). The own-from parser leavesis_groupdefaulted and deriveschatfromrecipient, so thechat-based guards are load-bearing (they keep a group/status/newsletter self-echo from being mis-classified).should_send_delivery_receipt(both the hot-pathClientcopy and thewacorecopy) now allows a self-fanout. A recipient-less own message (a self-note) stays skipped.build_delivery_receipt_nodeemitstype="sender"plus the device-strippedrecipient(matching WA WebUSER_JID), keepingtodevice-preserving (per fix(receipt): preserve sender device in delivery receiptto#649). A peer-synced message keepstype="peer_msg"and carries no recipient (WA Web!lguard): the sender shape is gated oncategory != Peer.handle_decrypt_failure— the path that actually fires for an account already stuck in the loop (its counter has advanced past the duplicate into BadMac): for a self-fanout it now clears the queue with the sender receipt and skips the retry-to-self, instead of the bare transport ack. Gated onis_self_fanout() && should_send_delivery_receipt()(same eligibility as the ack path).ReceiptType::as_wire_str()added and used for thesender/peer_msg/inactivetype attrs (no magic strings); also fixes thesend_delivery_receiptdebug label (it loggeddeliveryfor a passive companion'sinactivereceipt).encode_ack_bytesis unchanged (therecipienton the transport ack stays, per fix(client): preserve recipient in <ack>, soften unknown stream:error #633).Why skip the retry for a self-fanout decrypt failure
WA Web's
MsgSendReceiptRETRY branch sends a retry receipt, but it never hits that branch for an own fanout (it has the keys, so it never BadMacs its own message) — there is no canonical WA Web behavior to mirror here. Retrying our own already-sent content recovers nothing, and because the stuck stanza is redelivered only once per ~50min connection, retrying to the cap (5×) would prolong the disconnect loop ~4h. An immediate sender receipt clears it on the first cycle, and atype="sender"receipt for our own message is semantically truthful (we are the sender).Deliberate non-changes (verified against WA Web)
@bot-author DM to a user keeps the bare bot-invoke-response<ack>, not a sender receipt.MsgSendReceipt.jschecksh = !chat.isBot() && author.isBot()first and returnssendBotInvokeResponseAcks(a bare ack); only theelsereaches the SENDER receipt. The real bug case (own prompt to@bot,chat.server == Bot) makeshfalse and correctly falls through to the sender receipt. Locked byown_bot_author_dm_acks_not_sender_receipt. (Pre-existing nuance, out of scope: that bare ack omits thetype="text"WA Web emits, becauseencode_ack_bytesdropstypefortag=="message"; the@bot-server-author scenario is not real for normal accounts.)Tests
Happy + bad paths (all fictitious JIDs, no real PII):
bot_self_fanout_acked_via_sender_receipt— the exact symptom: own message to a@bot→<receipt type="sender" recipient=@bot>, device preserved into, and no bare<ack>.self_fanout_decrypt_failure_acked_via_sender_receipt— the BadMac/NoSession path (the one that fires for already-stuck accounts): sender receipt, no bare ack, no retry-to-self.own_self_fanout_acked_via_sender_receipt— user-recipient fanout (replaces the old test that asserted the buggy bare-ack behavior).own_bot_author_dm_acks_not_sender_receipt— locks the bot-author bare-ack ordering (codex P2 rejection) with a settle window.delivery_receipt_for_self_fanout_to_bot_is_sender_with_recipient/…_strips_recipient_device,peer_self_fanout_is_peer_msg_without_recipient,self_fanout_is_sender_even_when_inactive—build_delivery_receipt_nodewire shape + branch precedence.should_send_delivery_receipt_allows_self_fanout_to_{user,bot}/…_skips_own_status_and_group_fanout(+wacoreskip_own_status_and_group_even_with_recipientparity) — gate boundaries;…_skips_own_dmstill skips the recipient-less self-note.is_self_fanout_matches_only_own_dm_with_recipient(isolated guards incl.chat.is_group()withis_group=false) andas_wire_str_round_trips_through_parse.cargo test --workspace --exclude e2e-tests,cargo clippy --all-targets -- -D warnings, andcargo fmt --allall pass.Verification still needed (live)
The one premise that cannot be verified in-repo: that the server's offline GC drains on a
type="sender"receipt but ignores a bare ack. On a real account with a stuck bot self-fanout, confirm theofflinecount drops to 0 on the next reconnect and the ~50min loop stops.The
recipientvalue uses whatsmeow's per-stanza single-receipt form (recipient = the bot), which also matches WA Web's onlineDeliveryReceiptJob.js. WA Web's offline-aggregate job (OfflineDeliveryReceiptJob.js) instead groups byfromand emitsrecipient = self. Since this bug is an offline-redelivery loop, the offline-aggregate form is the most directly-relevant WA Web path — so the recipient form is the primary thing live testing must validate; switching torecipient = selfif required is a one-line change.