fix(offline): drain offline queue by acking duplicate and undecryptable messages - #647
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtends message-retry TTL to 1 hour, adds client receipt-activity state and best-effort transport ACK plumbing, centralizes decrypt-failure log-levels, orders retry receipts before optional transport ACKs (skip for status@broadcast), refactors PDO to awaitable run, and updates receipt node shape and tests. ChangesUndecryptable message acknowledgment and delivery receipt flow
Sequence DiagramsequenceDiagram
participant Client
participant Transport
participant Server
Client->>Client: dispatch_parsed_message / handle_decrypt_failure
Client->>Client: ack_received_message -> spawn_delivery_receipt / run_retry_receipt
alt not status@broadcast
Client->>Client: spawn_message_ack -> message_ack_source_node -> encode_ack_bytes
Client->>Transport: send_raw_bytes (best-effort ACK)
end
Client->>Server: delivery receipt (network)
Transport-->>Client: stream:error (may contain <ack/>)
Client->>Client: handle_stream_error (log ack class/id or warn)
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 |
![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: c0771952e3
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/client.rs`:
- Around line 1951-1974: spawn_message_ack currently spawns a detached task that
calls encode_ack_bytes and then client.send_raw_bytes directly, which can race
with disconnect()/reconnect() and bypass the outbound drain; change it to route
ACKs through the client's outbound_flush so the ACK is enqueued and drained
during teardown. Concretely, replace the runtime.spawn + direct send_raw_bytes
path in spawn_message_ack (the block that calls encode_ack_bytes and
send_raw_bytes) with code that encodes the ACK using
encode_ack_bytes/message_ack_source_node as before but then enqueues/schedules
the resulting buffer via the client's outbound_flush API (e.g., an enqueue or
push method on outbound_flush) instead of calling send_raw_bytes; preserve the
same logging for encode/send errors but ensure sending happens via
outbound_flush so disconnect/reconnect will not close the transport before the
ACK is written.
In `@src/message.rs`:
- Around line 753-760: The duplicate-delivery branch missing the status
broadcast guard causes delivery receipts to be sent for status@broadcast
stanzas; update the conditional that checks session_had_duplicates &&
!session_decrypted_successfully && !session_dispatched_undecryptable to also
ensure !is_status_broadcast(&info) before calling
self.spawn_delivery_receipt(&info) so status broadcast duplicates are skipped
(use the existing is_status_broadcast(&info) helper and the
spawn_delivery_receipt call to locate and change the code).
🪄 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: 7f9179fe-8eb2-4817-9b17-72c66e9d98eb
📒 Files selected for processing (3)
src/cache_config.rssrc/client.rssrc/message.rs
b0ae9bd to
2a0231c
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 31s —— View job Review: fix(offline): drain offline queue by acking duplicate and undecryptable messages
Overall: This is a well-structured fix for a real problem — un-acked offline stanzas replaying on every reconnect until the server force-closes the stream. The design is sound: every code path that receives a message now ensures the server gets a signal to drain it (delivery receipt, transport ack, or nack). The test coverage is thorough, with wire-level decode helpers that verify actual on-the-wire behavior. What looks good
Findings1. Stream error
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a0231c00d
ℹ️ 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message.rs (1)
456-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispatch the undecryptable event before scheduling PDO/ack.
This flips the ordering contract: the unavailable-stub path can start PDO / wire ack before consumers see
UndecryptableMessage, unlikehandle_decrypt_failure. That makes this branch racey in exactly the flow the new tests are trying to pin down.Suggested fix
- self.spawn_pdo_request_with_options(&info, true); - if !info.source.chat.is_status_broadcast() { - self.spawn_message_ack(&info); - } self.dispatch_undecryptable_event( Arc::clone(&info), true, unavailable_type, crate::types::events::DecryptFailMode::Show, ) .await; + self.spawn_pdo_request_with_options(&info, true); + if !info.source.chat.is_status_broadcast() { + self.spawn_message_ack(&info); + } return None;🤖 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 `@src/message.rs` around lines 456 - 469, The ordering is wrong: move the call to dispatch_undecryptable_event(...) so it runs before scheduling PDO/ack; specifically, in the branch that currently calls self.spawn_pdo_request_with_options(&info, true) and then optionally self.spawn_message_ack(&info), call self.dispatch_undecryptable_event(Arc::clone(&info), true, unavailable_type, crate::types::events::DecryptFailMode::Show).await first (matching handle_decrypt_failure's behavior), and only after that schedule spawn_pdo_request_with_options and, if !info.source.chat.is_status_broadcast(), spawn_message_ack, preserving the same arguments and behavior.
🤖 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 1371-1382: Currently the code skips calling
self.dispatch_undecryptable_event when info.source.chat.is_status_broadcast() is
true, which suppresses UndecryptableMessage events for status sender-key
failures; change the logic to always call
self.dispatch_undecryptable_event(Arc::clone(info), false,
crate::types::events::UnavailableType::Unknown, decrypt_fail_mode).await
unconditionally, and keep the spawn_nack(info, NackReason::UnhandledError, None)
call only inside a conditional gated by !info.source.chat.is_status_broadcast()
so status@broadcast still follows its special wire nack/ack behavior but
consumers receive the undecryptable event.
---
Outside diff comments:
In `@src/message.rs`:
- Around line 456-469: The ordering is wrong: move the call to
dispatch_undecryptable_event(...) so it runs before scheduling PDO/ack;
specifically, in the branch that currently calls
self.spawn_pdo_request_with_options(&info, true) and then optionally
self.spawn_message_ack(&info), call
self.dispatch_undecryptable_event(Arc::clone(&info), true, unavailable_type,
crate::types::events::DecryptFailMode::Show).await first (matching
handle_decrypt_failure's behavior), and only after that schedule
spawn_pdo_request_with_options and, if !info.source.chat.is_status_broadcast(),
spawn_message_ack, preserving the same arguments and behavior.
🪄 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: ede81ba8-2a24-4668-b7f4-3bc8ba237b84
📒 Files selected for processing (4)
src/cache_config.rssrc/client.rssrc/message.rssrc/receipt.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/message.rs (2)
1374-1385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep surfacing status sender-key failures to consumers.
This guard wraps both
dispatch_undecryptable_event()andspawn_nack(), so genericskmsgdecrypt failures onstatus@broadcastdisappear entirely. The status-specific exception is the wire nack, not the event.Suggested fix
- if !info.source.chat.is_status_broadcast() { - self.dispatch_undecryptable_event( - Arc::clone(info), - false, - crate::types::events::UnavailableType::Unknown, - decrypt_fail_mode, - ) - .await; - self.spawn_nack(info, NackReason::UnhandledError, None); - } + self.dispatch_undecryptable_event( + Arc::clone(info), + false, + crate::types::events::UnavailableType::Unknown, + decrypt_fail_mode, + ) + .await; + if !info.source.chat.is_status_broadcast() { + self.spawn_nack(info, NackReason::UnhandledError, None); + }🤖 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 `@src/message.rs` around lines 1374 - 1385, The current guard around info.source.chat.is_status_broadcast() prevents dispatch_undecryptable_event() from being called for status@broadcast messages; call dispatch_undecryptable_event(Arc::clone(info), false, crate::types::events::UnavailableType::Unknown, decrypt_fail_mode).await unconditionally so consumers still see sender-key failures, and keep the spawn_nack(info, NackReason::UnhandledError, None) call only inside the if !info.source.chat.is_status_broadcast() block so the wire nack remains status-specific.
774-781:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip the duplicate receipt on session-only
status@broadcastredeliveries.
ack_received_message()still emits a delivery receipt here becauseshould_send_delivery_receipt(info)is true for non-self status messages. Theskmsgduplicate path already guardsstatus@broadcast, so thismsg/pkmsg-only branch reintroduces the redundant status receipt.Suggested fix
- self.ack_received_message(&info); + if !info.source.chat.is_status_broadcast() { + self.ack_received_message(&info); + }🤖 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 `@src/message.rs` around lines 774 - 781, The duplicate-only branch calls self.ack_received_message(&info) which re-emits a delivery receipt for session-only status@broadcast redeliveries; update the conditional that checks session_had_duplicates && !session_decrypted_successfully && !session_dispatched_undecryptable to also skip acking when the message is a status@broadcast (i.e., add a guard like && !info.is_status_broadcast / && info.message_type != MessageType::StatusBroadcast or equivalent field on info) so ack_received_message(&info) is not invoked for status@broadcast redeliveries (note: preserve existing logic around should_send_delivery_receipt(info) and only prevent the redundant status receipt in this duplicate path).
🤖 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/client.rs`:
- Around line 4080-4101: The ACK source builder in message_ack_source_node
currently only uses info.source.is_group to pick the `from` JID, which misses
broadcast/status messages that must also ACK to `chat`; change the selection so
that `from` is info.source.chat when the stanza is a group OR a broadcast/status
(detect the broadcast/status flag on info.source or the message type alongside
info.source.is_group), keeping the existing recipient and participant attr
handling (builder.attr("recipient", ...), builder.attr("participant", ...))
unchanged, and add a regression test for a status@broadcast/undecryptable
message next to the new addressing tests to ensure transport ACKs are sent to
chat not sender.
---
Duplicate comments:
In `@src/message.rs`:
- Around line 1374-1385: The current guard around
info.source.chat.is_status_broadcast() prevents dispatch_undecryptable_event()
from being called for status@broadcast messages; call
dispatch_undecryptable_event(Arc::clone(info), false,
crate::types::events::UnavailableType::Unknown, decrypt_fail_mode).await
unconditionally so consumers still see sender-key failures, and keep the
spawn_nack(info, NackReason::UnhandledError, None) call only inside the if
!info.source.chat.is_status_broadcast() block so the wire nack remains
status-specific.
- Around line 774-781: The duplicate-only branch calls
self.ack_received_message(&info) which re-emits a delivery receipt for
session-only status@broadcast redeliveries; update the conditional that checks
session_had_duplicates && !session_decrypted_successfully &&
!session_dispatched_undecryptable to also skip acking when the message is a
status@broadcast (i.e., add a guard like && !info.is_status_broadcast / &&
info.message_type != MessageType::StatusBroadcast or equivalent field on info)
so ack_received_message(&info) is not invoked for status@broadcast redeliveries
(note: preserve existing logic around should_send_delivery_receipt(info) and
only prevent the redundant status receipt in this duplicate path).
🪄 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: c3d9ece5-1fc6-41e1-bbd5-1d831e5f2bd8
📒 Files selected for processing (4)
src/cache_config.rssrc/client.rssrc/message.rssrc/receipt.rs
2a0231c to
934bace
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 3m 0s —— View job Review: fix(offline): drain offline queue by acking duplicate and undecryptable messages
Overall: This is a solid, well-designed fix for a real production issue — un-acked offline stanzas replaying on every reconnect until the server force-closes the stream. Every code path that receives a message now ensures the server gets a signal to drain it. The test coverage is excellent, with wire-level frame decode helpers verifying actual on-the-wire behavior. What looks good
Findings1. Duplicate
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934bace945
ℹ️ 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".
| client.run_retry_receipt(&info, reason).await; | ||
| if !skip_ack { | ||
| client.send_transport_ack(&info).await; |
There was a problem hiding this comment.
When run_retry_receipt does not successfully put a resend request on the wire, this still sends the transport ack and clears the server's offline queue. That can happen when send_retry_receipt returns an error (for example missing device account when keys are required) or when MAX_DECRYPT_RETRIES is reached, where run_retry_receipt only spawns the PDO request detached and returns; during disconnect that PDO task can be skipped by the connection-shutdown signal while this flushed ack still lands. In those cases the original ciphertext is no longer redelivered and no peer/phone resend request was delivered, so the message can be lost; have run_retry_receipt report/await a successfully sent retry/PDO request and skip or delay this ack otherwise.
Useful? React with 👍 / 👎.
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 237-245: The PDO path (run_retry_receipt) must be awaited and
executed inside the same outbound_flush flush task before the transport ack so
the ack cannot flush and disconnect before PDO runs; replace detached spawns or
spawn_* that call run_retry_receipt with sequencing inside the
outbound_flush.spawn closure: first await client.run_retry_receipt(&info,
reason).await (or equivalent awaitable PDO function), then if !skip_ack await
client.send_transport_ack(&info).await. Apply the same change to the other
occurrences referenced (the block around lines 330-339 and the one at 470-473)
so PDO is always run and awaited inside the flush task prior to sending the
transport ack.
🪄 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: 03ec6e12-0944-4535-990a-bc191ecccbd8
📒 Files selected for processing (4)
src/cache_config.rssrc/client.rssrc/message.rssrc/receipt.rs
934bace to
38fb355
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38fb3553fd
ℹ️ 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".
| let skip_ack = info.source.chat.is_status_broadcast(); | ||
| self.outbound_flush.spawn(&*self.runtime, async move { | ||
| client.run_retry_receipt(&info, reason).await; | ||
| if !skip_ack { | ||
| client.send_transport_ack(&info).await; |
There was a problem hiding this comment.
For status@broadcast decrypt failures this suppresses the flushed transport ack and relies on the generic should_ack path after dispatch, but that path uses maybe_deferred_ack, which spawns a detached task rather than outbound_flush. If the client disconnects right after processing the failed status stanza, disconnect() can flush the retry task while the detached status ack is dropped, leaving the status message in the server offline queue to be replayed on reconnect. Send the status ack from the same flushed task (or make the generic ack path flushable) for this failure case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client.rs (1)
4123-4139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroadcast-list ACKs still route to the wrong JID.
Line 4124 only switches on
is_group. That covers status only if the parser marks it group-like, but regular broadcast-list sources (broadcast_list_owner.is_some()) still rebuildfromfromsender, so the transport ACK goes back to the participant instead of the chat and the offline stanza can keep replaying.Diff
fn message_ack_source_node(info: &crate::types::message::MessageInfo) -> Node { - let from = if info.source.is_group { + let from = if info.source.is_group || info.source.broadcast_list_owner.is_some() { &info.source.chat } else { &info.source.sender };🤖 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 `@src/client.rs` around lines 4123 - 4139, The ACK source selection in message_ack_source_node only checks info.source.is_group when choosing the "from" JID, causing broadcast-list messages (info.source.broadcast_list_owner.is_some()) to use sender instead of the chat; update the logic in message_ack_source_node to treat broadcast-list sources like groups when computing `from` (i.e., use &info.source.chat when info.source.is_group || info.source.broadcast_list_owner.is_some()), but keep the existing participant attribute handling (only set participant when info.source.is_group). Ensure the optional recipient attr behavior remains unchanged.
🤖 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/client.rs`:
- Around line 315-317: send_active_receipts (AtomicU32) currently carries
presence-driven state across reconnects; on connection teardown/start we must
reset the presence-driven value 1 back to 0 while preserving forced mode 2. In
the connection lifecycle code (e.g., the connect/disconnect or teardown/start
handlers where session state is reset) add an atomic small update on
send_active_receipts that only changes 1 -> 0 (use compare_exchange or
fetch_update on the AtomicU32) and leave any value 2 untouched; do this in both
the disconnect/teardown path and the connect/start path so presence state does
not leak across reconnections.
---
Duplicate comments:
In `@src/client.rs`:
- Around line 4123-4139: The ACK source selection in message_ack_source_node
only checks info.source.is_group when choosing the "from" JID, causing
broadcast-list messages (info.source.broadcast_list_owner.is_some()) to use
sender instead of the chat; update the logic in message_ack_source_node to treat
broadcast-list sources like groups when computing `from` (i.e., use
&info.source.chat when info.source.is_group ||
info.source.broadcast_list_owner.is_some()), but keep the existing participant
attribute handling (only set participant when info.source.is_group). Ensure the
optional recipient attr behavior remains 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: 5ced3a87-c6c4-4e1d-bfa7-1083c8bc7a67
📒 Files selected for processing (6)
src/cache_config.rssrc/client.rssrc/features/presence.rssrc/message.rssrc/pdo.rssrc/receipt.rs
38fb355 to
c5ba746
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5ba746ba3
ℹ️ 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".
| self.send_active_receipts | ||
| .store(if active { 2 } else { 0 }, Ordering::Release); |
There was a problem hiding this comment.
When set_force_active_delivery_receipts(false) is called while the client is already available, this unconditionally stores 0, so subsequent delivery receipts are sent as type="inactive" even though presence is still active. The state machine already distinguishes presence-active (1) from forced (2), so clearing the force should not drop an active-presence state; otherwise any temporary force toggle during an online session suppresses normal active receipts until presence is resent.
Useful? React with 👍 / 👎.
| PresenceStatus::Available => { | ||
| self.client.send_unified_session().await; | ||
| self.client.mark_receipts_active_on_presence(); | ||
| } | ||
| PresenceStatus::Unavailable => self.client.mark_receipts_inactive_on_presence(), |
There was a problem hiding this comment.
In the PresenceStatus::Available path this marks receipts active before the presence stanza is successfully sent; if send_node returns a transport error, the method reports failure but future delivery receipts are still emitted as active even though the server never accepted the available presence. This affects failed/retrying presence changes, and the same stale-state problem exists in the unavailable path; move the state transition after a successful send or roll it back on error.
Useful? React with 👍 / 👎.
Duplicate, undecryptable, unavailable and own-account fan-out messages were acknowledged inconsistently, so the server replayed them on every reconnect until it force-closed the stream. Ensure every received message is acked: - Ack duplicates and own-account fan-out (own non-peer gets a transport ack with the correct to=from, since its delivery receipt is suppressed). - Send a transport ack on decrypt failure and on <unavailable>; the retry receipt alone does not clear the offline queue. - Nack generic group decrypt errors, matching the session catch-all. - Send the retry receipt, PDO request and transport ack in one ordered, flushed task so a disconnect can't clear a stanza before its resend request (retry/PDO) goes out. - Send delivery receipts as type="inactive" unless marked online, matching whatsmeow's background-companion behavior. - Raise message_retry_counts TTL to 1h so the retry cap survives spaced redeliveries. - Lower decrypt-fail="hide" failures to DEBUG. - Recognize stream:error with an ack child instead of logging it as unknown.
c5ba746 to
73085eb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73085eb680
ℹ️ 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".
| let skip_ack = info.source.chat.is_status_broadcast(); | ||
| self.outbound_flush.spawn(&*self.runtime, async move { | ||
| // Only ack once the PDO request is out (or skipped as ancient); | ||
| // a transient send failure leaves it queued for redelivery. | ||
| let pdo_sent = client.run_pdo_request(&info2).await; | ||
| if !skip_ack && pdo_sent { | ||
| client.send_transport_ack(&info2).await; |
There was a problem hiding this comment.
For status@broadcast stanzas that contain only <unavailable>, this skip_ack path avoids the ordered/flushed ack and relies on the generic should_ack path after the handler returns. That generic ack is spawned independently, so it can be sent during disconnect or simply race ahead of run_pdo_request; if it clears the offline stanza before the PDO request is successfully sent, the placeholder can no longer be recovered or redelivered. Treat status the same as non-status here (or otherwise suppress/order the generic ack) so the ack only goes out after the PDO request succeeds.
Useful? React with 👍 / 👎.
…epair e2e Codex P2: the unknown-enc fallback was reconstructing the ack from MessageInfo, but parse_message_info only retains `recipient` on the self-sent branch, so LID-routed / hosted-companion / peer stanzas lost the attribute and the server replied <stream:error><ack/>. Add `Client::spawn_node_transport_ack` which encodes from the original NodeRef so `recipient` is echoed verbatim, and use it in the classify fallback. Outside-diff comment: `EncPayload::from_owned_node` returns None for both unknown types and known-but-empty content. Resolve `EncType::from_wire` up front and only set `had_unknown_enc` when the type itself is unrecognized; known-but-empty (malformed) falls back through the regular flow. Repair the e2e regression introduced by PR #647: `with_push_name` pre-seeds the name, so the setting_pushName mutation arrives with `old == new` and skips the auto `presence().set_available()` that would flip send_active_receipts to 1. As a result delivery receipts went out as type="inactive" and `test_bidirectional_offline_receipt` / `test_deferred_delivery_receipt_on_reconnect` failed in CI. Force active receipts in `TestClient::connect_inner` (mirrors whatsmeow's `SetForceActiveDeliveryReceipts`). Tests: - `unknown_only_enc_ack_preserves_recipient` for the LID-routed ack shape. - `known_enc_type_with_empty_content_skips_fallback_ack` for the new gate.
1 participant
Summary
Every received message is now acknowledged so the server drains it from the offline queue instead of replaying it on each reconnect (which eventually force-closed the stream).
to=from) since their delivery receipt is suppressed.<unavailable>; the retry receipt alone does not clear the queue.type="inactive"unless marked online (presence available or forced), matching whatsmeow's background companion.message_retry_countsTTL to 1h so the retry cap survives spaced redeliveries.decrypt-fail="hide"failures to DEBUG.stream:errorcarrying anackchild instead of logging it as unknown.Test plan
cargo test --workspace --exclude e2e-testscargo clippy --all --testscargo fmt --all --check