perf(receipt): aggregate offline delivery receipts per chat like WA Web - #820
Merged
Merged
Conversation
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR buffers delivery receipts during offline sync, computes an optional receipt ChangesOffline Receipt Buffering and Aggregation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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: 77aceb62b9
ℹ️ 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 Results67 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: 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/lifecycle.rs`:
- Around line 492-495: Update the comment in disconnect() to accurately describe
buffering behavior: explain that disconnect() calls flush_offline_receipts()
which drains current buffered receipts but does not flip offline_sync_completed,
so try_buffer_offline_receipt() can still add new receipts to
offline_receipt_buffer after the drain (because outbound_flush.close() only
stops spawning outbound tasks), and those later receipts will be handled on the
next flush (e.g., reconnect()/reconnect_immediately()) rather than within the
disconnect call; reference disconnect(), flush_offline_receipts(),
complete_offline_sync(), try_buffer_offline_receipt(), outbound_flush.close(),
offline_sync_completed, and offline_receipt_buffer in the updated comment.
In `@src/receipt.rs`:
- Around line 561-574: The try_buffer_offline_receipt function currently calls
.expect("offline receipt buffer poisoned") on offline_receipt_buffer which
panics on mutex poison and violates the no-.expect/.unwrap rule; replace that
panic with graceful handling: acquire the lock with .lock() and handle Err by
either returning false (fallback to 1:1 send) or mapping the poisoned guard via
.into_inner() to continue, or change the signature to return a Result<bool,
YourError> and propagate the locking error; update the code paths around
offline_receipt_buffer, try_buffer_offline_receipt, and checks of
offline_sync_completed accordingly, or if you intentionally want fail-fast,
replace the .expect with a short comment on why poison should panic and keep
behavior consistent.
🪄 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: 25a95eff-6612-4dcd-8fb7-7fa3b17dba0a
📒 Files selected for processing (5)
src/client.rssrc/client/lifecycle.rssrc/client/sessions.rssrc/message/dispatch.rssrc/receipt.rs
A receipt buffered after disconnect()'s drain (close() only stops outbound spawns, not buffering) would otherwise leak into the next connection's aggregate flush. The connection-state resets now clear the buffer; the server redelivers those unacked messages on the next connect. Also use the poison-recovery lock pattern instead of expect(), and make the disconnect drain comment precise.
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
Every received message emits its own delivery
<receipt>: one node build, one marshal, one Noise frame, one socket write. During an offline-sync reconnect the server drains the backlog in batches of 200, so a few hundred queued messages produce a few hundred receipt stanzas in a burst. WA Web optimizes exactly this case:sendAggregateOfflineReceiptsgroups the backlog's receipts by chat and author and emits ONE<receipt>per group, with the first message id as theidattr and the rest as<list><item id=.../></list>children, chunked at 256 ids. Our ingest already parses that aggregated shape (collect_simple_message_ids); we just never produced it.Change
Offline-drained messages (
info.is_offline, set from the stanza'sofflineattr) buffer their delivery receipt instead of sending it 1:1, andcomplete_offline_syncflushes the buffer as aggregate receipts. Live messages keep the existing 1:1 path behind a single bool check, matching WA Web sending live receipts individually.Arc<MessageInfo>clones (refcount bumps, nothing copied). Grouping keys are fully borrowed (&Jid+ a static type str), and message ids enter the nodes as&strborrows, so the flush allocates only the per-group vectors and the nodes themselves.delivery_receipt_typeis the single source of truth for the type attr in both the builder and the grouping key, so they cannot drift. Splitting finer than WA Web (e.g. on recipient device) is always wire-safe; merging across any of these would corrupt the receipt.sendAggregateReceipts:id= first id, remaining ids in<list><item>, the type attr dropped for plain delivery,tcarrying the flush timestamp,participantonly for group/status chats, 256-id chunks.complete_offline_syncflips the completed flag before taking the buffer lock, and producers re-check the flag under the lock, so a late offline receipt either lands in the flush or falls back to 1:1.disconnect/reconnect/reconnect_immediatelydrain the buffer into theoutbound_flushwindow before closing it, preserving the issue Delivery receipts may be dropped if disconnect() is called shortly after a message is received #571 flush semantics.mem::take, so outside the offline window the buffer is an empty Vec with zero capacity (asserted in a test). No memory is held between offline windows.Benchmark
Temporary measurement (release, 100 runs, bench not committed): a 200-message offline backlog from one chat.
The dominant saving is not in the table: 199 fewer Noise frame encryptions and 199 fewer socket writes per 200-message chat backlog, plus the matching reduction on the server side.
Tests
aggregate_delivery_receipts_group_by_chat_author_and_type: DM receipts coalesce, group authors split,peer_msgnever merges into the plain delivered group.aggregate_delivery_receipt_node_shape_and_ingest_roundtrip: asserts the WA Web wire shape and round-trips the produced node through our own ingest parser (collect_simple_message_ids).aggregate_delivery_receipt_chunks_at_256_ids: 257 ids split into 256 + 1, and a single-id chunk carries no empty<list>.offline_receipt_buffer_protocol: end-to-end buffer lifecycle including the late-receipt fallback after completion and the zero-capacity assertion after the drain.cargo fmt --allcargo clippy --all-targets -- -D warningscargo test -p whatsapp-rust --lib(757 passing)cargo test -p wacore(994 passing)Breaking
None. The public API is unchanged; live receipts keep the exact same wire shape and timing.