perf: remove four per-message tasks and buffers from the send/receive round trip - #1116
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR unifies IQ and phash response waiters, adds epoch-based phash cleanup, queues deferred transport ACKs through persistent workers, refactors secret-buffer draining, streams message-ID hashing, expands tests and memory reporting, and ignores generated snapshot artifacts. ChangesRuntime waiter and queue flows
Secret-buffer drain worker
Message-ID generation
Test artifact hygiene
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| src/client/node_io.rs | Adds the shared deferred-ack worker and performs phash comparisons while routing incoming acknowledgements. |
| src/client.rs | Introduces typed IQ/phash waiters, epoch-based phash cleanup, ack-worker state, and queue metrics. |
| src/send/mod.rs | Registers phash expectations before sending and retains asynchronous work only for the mismatch path. |
| src/msg_secret_buffer.rs | Replaces repeated drain tasks with a weakly owned, capacity-one-woken worker while preserving terminal inline flushing. |
| src/keepalive.rs | Sweeps unanswered phash waiters on every keepalive tick before activity-based early returns. |
| wacore/src/request.rs | Generates message IDs without a staging vector or newly seeded per-message RNG. |
Sequence Diagram
sequenceDiagram
participant Send as Send pipeline
participant Waiters as Response waiter map
participant Server as WhatsApp server
participant Read as Read loop
participant Cache as Device/group caches
Send->>Waiters: Register expected phash
Send->>Server: Send encrypted stanza
Server-->>Read: Ack with phash
Read->>Waiters: Remove waiter by message ID
alt phash matches
Read-->>Send: Complete inline
else phash differs
Read->>Cache: Spawn cache invalidation
end
loop Keepalive tick
Read->>Waiters: Sweep expired phash waiters
end
Reviews (6): Last reviewed commit: "fix(voip): gate the node-returning ack w..." | Re-trigger Greptile
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/keepalive.rs (1)
81-93: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe sweep never runs on a busy connection — phash waiters leak for the life of the socket.
This is the part that has to work right. The sweep lives inside
send_keepalive, butkeepalive_loopreturns early at Line 179 whenever data arrived withinKEEP_ALIVE_INTERVAL_MIN— itcontinues before Line 196 ever callssend_keepalive. So on any connection carrying real traffic,drop_expired_phashis never invoked.Every DM/group/status send now registers a phash waiter (
src/send/mod.rsLine 1766 and Line 1236). Each one whose ack is lost stays inresponse_waitersforever. On the exact 120k-message workload this PR benchmarks, the recent-activity early-return fires on every tick, so the map grows monotonically — unbounded memory plus a wideningstd::sync::Mutexcritical section on the send hot path. That trades the allocation win straight back.Hoist the sweep into the tick body, above the early-return, next to
spawn_retention_cleanupwhich is already positioned there for this same reason.🐛 Proposed fix: sweep on every tick, not only when a ping is sent
In
send_keepalive, drop the sweep and keep the plain check:// WA Web: skip ping if there are pending IQs // (`activePing || ackHandlers.length || pendingIqs.size`) - // - // Sweep first: a phash waiter is resolved by an ack that may never come, - // and nothing else polls it. Leaving one behind would read as "IQ - // pending" and silence keepalives for the life of the connection. - let has_pending = { - let mut waiters = self.response_waiters_guard(); - waiters.drop_expired_phash(wacore::time::now_secs()); - !waiters.is_empty() - }; - if has_pending { + if !self.response_waiters_guard().is_empty() { debug!(target: "Client/Keepalive", "Skipping ping: IQ responses pending"); return KeepaliveResult::Ok; }Then run it on every tick in
keepalive_loop, before the recent-activity early-return:cleanup_counter += 1; if cleanup_counter >= 12 { cleanup_counter = 0; self.spawn_retention_cleanup(sent_msg_ttl); } + // A phash waiter is resolved by an ack that may never come, and + // nothing else polls it. Sweep on the tick, not inside the ping: + // a busy connection skips the ping entirely and would otherwise + // accumulate a waiter per lost ack for the life of the socket. + self.response_waiters_guard() + .drop_expired_phash(wacore::time::now_secs()); + let last_recv = self.stats.last_data_received_ms();🤖 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/keepalive.rs` around lines 81 - 93, Move the expired-phash waiter sweep out of send_keepalive and into keepalive_loop’s per-tick path, immediately before the recent-activity early return and alongside spawn_retention_cleanup. Keep send_keepalive limited to checking whether response waiters remain, preserving its existing pending-IQ behavior while ensuring drop_expired_phash runs on every tick.
🤖 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 735-737: Rename the ResponseWaiterEntry.sender field to waiter and
update every construction, access, pattern, and destructuring site, including
entry.sender in drop_expired_phash, so no stale sender references remain.
---
Outside diff comments:
In `@src/keepalive.rs`:
- Around line 81-93: Move the expired-phash waiter sweep out of send_keepalive
and into keepalive_loop’s per-tick path, immediately before the recent-activity
early return and alongside spawn_retention_cleanup. Keep send_keepalive limited
to checking whether response waiters remain, preserving its existing pending-IQ
behavior while ensuring drop_expired_phash runs on every tick.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7555878f-9cf0-4058-a08e-21384950b32e
📒 Files selected for processing (11)
src/client.rssrc/client/lifecycle.rssrc/client/messaging.rssrc/client/node_io.rssrc/client/tests.rssrc/keepalive.rssrc/msg_secret_buffer.rssrc/request.rssrc/send/mod.rssrc/test_utils.rswacore/src/request.rs
![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: da91a8b6cf
ℹ️ 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".
| expected, | ||
| jid, | ||
| invalidate_group_cache, | ||
| expires_at_secs: sent_at_secs + PHASH_ACK_TIMEOUT_SECS, |
There was a problem hiding this comment.
When stanza preparation or the pre-wire persistence flush takes close to ten seconds, this deadline is already stale when the waiter is registered because sent_at_secs was sampled at the beginning of send_message_impl; a concurrent keepalive tick can therefore remove the waiter immediately, causing a subsequent phash-mismatching ack to skip the required device/group cache invalidation. It is also a wall-clock deadline even though wacore/src/time.rs explicitly permits that clock to jump, so clock adjustments can expire live waiters or retain lost-ack waiters. Base the timeout on a monotonic instant captured when the waiter is inserted, matching the previous timer's ten seconds after sending.
Useful? React with 👍 / 👎.
| let has_pending = { | ||
| let mut waiters = self.response_waiters_guard(); | ||
| waiters.drop_expired_phash(wacore::time::now_secs()); |
There was a problem hiding this comment.
When the connection continuously receives traffic at least once every 15 seconds, keepalive_loop takes its recent-activity continue before calling send_keepalive, so this is never reached. Any outgoing-message ack lost during that period therefore leaves its phash entry in response_waiters indefinitely, and repeated lost acks make the map grow for as long as inbound traffic remains active; move the sweep to the periodic loop before the recent-activity early return.
Useful? React with 👍 / 👎.
| pub(crate) transport_ack_queue: std::sync::OnceLock< | ||
| async_channel::Sender<( | ||
| Arc<wacore_binary::OwnedNodeRef>, | ||
| crate::flush_scope::FlushGuard, | ||
| )>, |
There was a problem hiding this comment.
When the transport is slow or stalled, this unbounded queue can retain many full inbound nodes and flush guards, but Client::memory_report() does not count either its entries or retained bytes. That makes the report understate precisely the growth introduced by the new persistent worker and prevents the documented per-session leak diagnostics from identifying an ack backlog; expose the queue length and estimated node retention in MemoryReport.
AGENTS.md reference: AGENTS.md:L54-L54
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 `@memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST`:
- Around line 68-71: Remove the diagnostic SQLite artifacts
memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST (lines
68-71), memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST
(lines 68-71), and
memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY
(lines 68-71) from version control, then add the memdb_*.snapshot-* pattern to
.gitignore. If persisted-state assertions are needed, update them to compare
queried, normalized schema and row projections rather than raw SQLite images;
PersistenceManager::create_snapshot requires no direct 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47dfc13b-9d5b-46be-b335-fc1f52ee0197
📒 Files selected for processing (105)
file:memdb_badmac_lid_shadow_262_1745544.snapshot-1785024781-pre_pkmsg_.jsonfile:memdb_badmac_lid_shadow_262_1748392.snapshot-1785024800-pre_pkmsg_.jsonfile:memdb_badmac_preserves_263_1745544.snapshot-1785024781-pre_pkmsg_.jsonfile:memdb_badmac_preserves_263_1748392.snapshot-1785024800-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_closed_flush_1_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_closed_flush_1_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_offline_3_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_offline_3_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_prepare_retry_0_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_prepare_retry_0_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_retry_2_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_retry_2_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_bad_plaintext_no_skdm_ack_75_1745544.snapshot-1785024781-pre_pkmsg_BAD_SESSION_PLAINTEXT.jsonfile:memdb_capt_bad_plaintext_no_skdm_ack_75_1748392.snapshot-1785024800-pre_pkmsg_BAD_SESSION_PLAINTEXT.jsonfile:memdb_capt_bad_session_skips_skmsg_4_1745544.snapshot-1785024780-pre_pkmsg_BAD_SESSION_WITH_SKMSG.jsonfile:memdb_capt_bad_session_skips_skmsg_4_1748392.snapshot-1785024799-pre_pkmsg_BAD_SESSION_WITH_SKMSG.jsonfile:memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST.jsonfile:memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST.jsonfile:memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY.jsonfile:memdb_capt_group_skmsg_lock_happy_31_1748392.snapshot-1785024799-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY.jsonfile:memdb_capt_migration_plaintext_nack_44_1745544.snapshot-1785024781-pre_pkmsg_.jsonfile:memdb_capt_migration_plaintext_nack_44_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_mixed_skdm_bad_plaintext_47_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_BAD_SESSION.jsonfile:memdb_capt_mixed_skdm_bad_plaintext_47_1748392.snapshot-1785024799-pre_pkmsg_SKDM_WITH_BAD_SESSION.jsonfile:memdb_capt_own_group_skdm_ack_63_1745544.snapshot-1785024781-pre_pkmsg_OWN_GROUP_SKDM_ONLY.jsonfile:memdb_capt_own_group_skdm_ack_63_1748392.snapshot-1785024800-pre_pkmsg_OWN_GROUP_SKDM_ONLY.jsonfile:memdb_capt_pkmsg_parse_nack_65_1745544.snapshot-1785024781-pre_pkmsg_REGRESSION_PKMSG_PARSE.jsonfile:memdb_capt_pkmsg_parse_nack_65_1748392.snapshot-1785024800-pre_pkmsg_REGRESSION_PKMSG_PARSE.jsonfile:memdb_capt_session_content_group_ack_74_1745544.snapshot-1785024781-pre_pkmsg_SESSION_CONTENT_GROUP.jsonfile:memdb_capt_session_content_group_ack_74_1748392.snapshot-1785024800-pre_pkmsg_SESSION_CONTENT_GROUP.jsonfile:memdb_capt_skdm_msmsg_no_fallback_ack_78_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_MSMSG.jsonfile:memdb_capt_skdm_msmsg_no_fallback_ack_78_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_MSMSG.jsonfile:memdb_capt_skdm_only_group_ack_77_1745544.snapshot-1785024781-pre_pkmsg_SKDM_ONLY_SESSION.jsonfile:memdb_capt_skdm_only_group_ack_77_1748392.snapshot-1785024800-pre_pkmsg_SKDM_ONLY_SESSION.jsonfile:memdb_capt_skdm_plus_skmsg_ack_79_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_SKMSG.jsonfile:memdb_capt_skdm_plus_skmsg_ack_79_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_SKMSG.jsonfile:memdb_capt_status_skdm_only_ack_81_1745544.snapshot-1785024781-pre_pkmsg_STATUS_SKDM_ONLY.jsonfile:memdb_capt_status_skdm_only_ack_81_1748392.snapshot-1785024800-pre_pkmsg_STATUS_SKDM_ONLY.jsonfile:memdb_invalid_spk_id_264_1745544.snapshot-1785024781-pre_pkmsg_INVALID_SPK_ID_MSG.jsonfile:memdb_invalid_spk_id_264_1748392.snapshot-1785024800-pre_pkmsg_INVALID_SPK_ID_MSG.jsonfile:memdb_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789.jsonfile:memdb_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789.jsonfile:memdb_no_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789.jsonfile:memdb_no_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789.jsonfile:memdb_prod_archive_265_1745544.snapshot-1785024782-pre_pkmsg_.jsonfile:memdb_prod_archive_265_1748392.snapshot-1785024801-pre_pkmsg_.jsonfile:memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST0.jsonfile:memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST1.jsonfile:memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST2.jsonfile:memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST0.jsonfile:memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST1.jsonfile:memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST2.jsonmemdb_badmac_lid_shadow_262_1745544.snapshot-1785024781-pre_pkmsg_memdb_badmac_lid_shadow_262_1748392.snapshot-1785024800-pre_pkmsg_memdb_badmac_preserves_263_1745544.snapshot-1785024781-pre_pkmsg_memdb_badmac_preserves_263_1748392.snapshot-1785024800-pre_pkmsg_memdb_capt_appstate_key_share_closed_flush_1_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_closed_flush_1_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_appstate_key_share_offline_3_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_offline_3_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_appstate_key_share_prepare_retry_0_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_prepare_retry_0_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_appstate_key_share_retry_2_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_retry_2_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_bad_plaintext_no_skdm_ack_75_1745544.snapshot-1785024781-pre_pkmsg_BAD_SESSION_PLAINTEXTmemdb_capt_bad_plaintext_no_skdm_ack_75_1748392.snapshot-1785024800-pre_pkmsg_BAD_SESSION_PLAINTEXTmemdb_capt_bad_session_skips_skmsg_4_1745544.snapshot-1785024780-pre_pkmsg_BAD_SESSION_WITH_SKMSGmemdb_capt_bad_session_skips_skmsg_4_1748392.snapshot-1785024799-pre_pkmsg_BAD_SESSION_WITH_SKMSGmemdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_ESTmemdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_ESTmemdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPYmemdb_capt_group_skmsg_lock_happy_31_1748392.snapshot-1785024799-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPYmemdb_capt_migration_plaintext_nack_44_1745544.snapshot-1785024781-pre_pkmsg_memdb_capt_migration_plaintext_nack_44_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_mixed_skdm_bad_plaintext_47_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_BAD_SESSIONmemdb_capt_mixed_skdm_bad_plaintext_47_1748392.snapshot-1785024799-pre_pkmsg_SKDM_WITH_BAD_SESSIONmemdb_capt_own_group_skdm_ack_63_1745544.snapshot-1785024781-pre_pkmsg_OWN_GROUP_SKDM_ONLYmemdb_capt_own_group_skdm_ack_63_1748392.snapshot-1785024800-pre_pkmsg_OWN_GROUP_SKDM_ONLYmemdb_capt_pkmsg_parse_nack_65_1745544.snapshot-1785024781-pre_pkmsg_REGRESSION_PKMSG_PARSEmemdb_capt_pkmsg_parse_nack_65_1748392.snapshot-1785024800-pre_pkmsg_REGRESSION_PKMSG_PARSEmemdb_capt_session_content_group_ack_74_1745544.snapshot-1785024781-pre_pkmsg_SESSION_CONTENT_GROUPmemdb_capt_session_content_group_ack_74_1748392.snapshot-1785024800-pre_pkmsg_SESSION_CONTENT_GROUPmemdb_capt_skdm_msmsg_no_fallback_ack_78_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_MSMSGmemdb_capt_skdm_msmsg_no_fallback_ack_78_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_MSMSGmemdb_capt_skdm_only_group_ack_77_1745544.snapshot-1785024781-pre_pkmsg_SKDM_ONLY_SESSIONmemdb_capt_skdm_only_group_ack_77_1748392.snapshot-1785024800-pre_pkmsg_SKDM_ONLY_SESSIONmemdb_capt_skdm_plus_skmsg_ack_79_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_SKMSGmemdb_capt_skdm_plus_skmsg_ack_79_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_SKMSGmemdb_capt_status_skdm_only_ack_81_1745544.snapshot-1785024781-pre_pkmsg_STATUS_SKDM_ONLYmemdb_capt_status_skdm_only_ack_81_1748392.snapshot-1785024800-pre_pkmsg_STATUS_SKDM_ONLYmemdb_invalid_spk_id_264_1745544.snapshot-1785024781-pre_pkmsg_INVALID_SPK_ID_MSGmemdb_invalid_spk_id_264_1748392.snapshot-1785024800-pre_pkmsg_INVALID_SPK_ID_MSGmemdb_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789memdb_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789memdb_no_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789memdb_no_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789memdb_prod_archive_265_1745544.snapshot-1785024782-pre_pkmsg_memdb_prod_archive_265_1748392.snapshot-1785024801-pre_pkmsg_memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST0memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST1memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST2memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST0memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST1memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST2src/client/messaging.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST`:
- Around line 68-71: Remove the diagnostic SQLite artifacts
memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST (lines
68-71), memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST
(lines 68-71), and
memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY
(lines 68-71) from version control, then add the memdb_*.snapshot-* pattern to
.gitignore. If persisted-state assertions are needed, update them to compare
queried, normalized schema and row projections rather than raw SQLite images;
PersistenceManager::create_snapshot requires no direct 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47dfc13b-9d5b-46be-b335-fc1f52ee0197
📒 Files selected for processing (105)
file:memdb_badmac_lid_shadow_262_1745544.snapshot-1785024781-pre_pkmsg_.jsonfile:memdb_badmac_lid_shadow_262_1748392.snapshot-1785024800-pre_pkmsg_.jsonfile:memdb_badmac_preserves_263_1745544.snapshot-1785024781-pre_pkmsg_.jsonfile:memdb_badmac_preserves_263_1748392.snapshot-1785024800-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_closed_flush_1_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_closed_flush_1_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_offline_3_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_offline_3_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_prepare_retry_0_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_prepare_retry_0_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_retry_2_1745544.snapshot-1785024780-pre_pkmsg_.jsonfile:memdb_capt_appstate_key_share_retry_2_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_bad_plaintext_no_skdm_ack_75_1745544.snapshot-1785024781-pre_pkmsg_BAD_SESSION_PLAINTEXT.jsonfile:memdb_capt_bad_plaintext_no_skdm_ack_75_1748392.snapshot-1785024800-pre_pkmsg_BAD_SESSION_PLAINTEXT.jsonfile:memdb_capt_bad_session_skips_skmsg_4_1745544.snapshot-1785024780-pre_pkmsg_BAD_SESSION_WITH_SKMSG.jsonfile:memdb_capt_bad_session_skips_skmsg_4_1748392.snapshot-1785024799-pre_pkmsg_BAD_SESSION_WITH_SKMSG.jsonfile:memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST.jsonfile:memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST.jsonfile:memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY.jsonfile:memdb_capt_group_skmsg_lock_happy_31_1748392.snapshot-1785024799-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY.jsonfile:memdb_capt_migration_plaintext_nack_44_1745544.snapshot-1785024781-pre_pkmsg_.jsonfile:memdb_capt_migration_plaintext_nack_44_1748392.snapshot-1785024799-pre_pkmsg_.jsonfile:memdb_capt_mixed_skdm_bad_plaintext_47_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_BAD_SESSION.jsonfile:memdb_capt_mixed_skdm_bad_plaintext_47_1748392.snapshot-1785024799-pre_pkmsg_SKDM_WITH_BAD_SESSION.jsonfile:memdb_capt_own_group_skdm_ack_63_1745544.snapshot-1785024781-pre_pkmsg_OWN_GROUP_SKDM_ONLY.jsonfile:memdb_capt_own_group_skdm_ack_63_1748392.snapshot-1785024800-pre_pkmsg_OWN_GROUP_SKDM_ONLY.jsonfile:memdb_capt_pkmsg_parse_nack_65_1745544.snapshot-1785024781-pre_pkmsg_REGRESSION_PKMSG_PARSE.jsonfile:memdb_capt_pkmsg_parse_nack_65_1748392.snapshot-1785024800-pre_pkmsg_REGRESSION_PKMSG_PARSE.jsonfile:memdb_capt_session_content_group_ack_74_1745544.snapshot-1785024781-pre_pkmsg_SESSION_CONTENT_GROUP.jsonfile:memdb_capt_session_content_group_ack_74_1748392.snapshot-1785024800-pre_pkmsg_SESSION_CONTENT_GROUP.jsonfile:memdb_capt_skdm_msmsg_no_fallback_ack_78_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_MSMSG.jsonfile:memdb_capt_skdm_msmsg_no_fallback_ack_78_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_MSMSG.jsonfile:memdb_capt_skdm_only_group_ack_77_1745544.snapshot-1785024781-pre_pkmsg_SKDM_ONLY_SESSION.jsonfile:memdb_capt_skdm_only_group_ack_77_1748392.snapshot-1785024800-pre_pkmsg_SKDM_ONLY_SESSION.jsonfile:memdb_capt_skdm_plus_skmsg_ack_79_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_SKMSG.jsonfile:memdb_capt_skdm_plus_skmsg_ack_79_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_SKMSG.jsonfile:memdb_capt_status_skdm_only_ack_81_1745544.snapshot-1785024781-pre_pkmsg_STATUS_SKDM_ONLY.jsonfile:memdb_capt_status_skdm_only_ack_81_1748392.snapshot-1785024800-pre_pkmsg_STATUS_SKDM_ONLY.jsonfile:memdb_invalid_spk_id_264_1745544.snapshot-1785024781-pre_pkmsg_INVALID_SPK_ID_MSG.jsonfile:memdb_invalid_spk_id_264_1748392.snapshot-1785024800-pre_pkmsg_INVALID_SPK_ID_MSG.jsonfile:memdb_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789.jsonfile:memdb_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789.jsonfile:memdb_no_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789.jsonfile:memdb_no_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789.jsonfile:memdb_prod_archive_265_1745544.snapshot-1785024782-pre_pkmsg_.jsonfile:memdb_prod_archive_265_1748392.snapshot-1785024801-pre_pkmsg_.jsonfile:memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST0.jsonfile:memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST1.jsonfile:memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST2.jsonfile:memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST0.jsonfile:memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST1.jsonfile:memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST2.jsonmemdb_badmac_lid_shadow_262_1745544.snapshot-1785024781-pre_pkmsg_memdb_badmac_lid_shadow_262_1748392.snapshot-1785024800-pre_pkmsg_memdb_badmac_preserves_263_1745544.snapshot-1785024781-pre_pkmsg_memdb_badmac_preserves_263_1748392.snapshot-1785024800-pre_pkmsg_memdb_capt_appstate_key_share_closed_flush_1_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_closed_flush_1_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_appstate_key_share_offline_3_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_offline_3_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_appstate_key_share_prepare_retry_0_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_prepare_retry_0_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_appstate_key_share_retry_2_1745544.snapshot-1785024780-pre_pkmsg_memdb_capt_appstate_key_share_retry_2_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_bad_plaintext_no_skdm_ack_75_1745544.snapshot-1785024781-pre_pkmsg_BAD_SESSION_PLAINTEXTmemdb_capt_bad_plaintext_no_skdm_ack_75_1748392.snapshot-1785024800-pre_pkmsg_BAD_SESSION_PLAINTEXTmemdb_capt_bad_session_skips_skmsg_4_1745544.snapshot-1785024780-pre_pkmsg_BAD_SESSION_WITH_SKMSGmemdb_capt_bad_session_skips_skmsg_4_1748392.snapshot-1785024799-pre_pkmsg_BAD_SESSION_WITH_SKMSGmemdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_ESTmemdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_ESTmemdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPYmemdb_capt_group_skmsg_lock_happy_31_1748392.snapshot-1785024799-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPYmemdb_capt_migration_plaintext_nack_44_1745544.snapshot-1785024781-pre_pkmsg_memdb_capt_migration_plaintext_nack_44_1748392.snapshot-1785024799-pre_pkmsg_memdb_capt_mixed_skdm_bad_plaintext_47_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_BAD_SESSIONmemdb_capt_mixed_skdm_bad_plaintext_47_1748392.snapshot-1785024799-pre_pkmsg_SKDM_WITH_BAD_SESSIONmemdb_capt_own_group_skdm_ack_63_1745544.snapshot-1785024781-pre_pkmsg_OWN_GROUP_SKDM_ONLYmemdb_capt_own_group_skdm_ack_63_1748392.snapshot-1785024800-pre_pkmsg_OWN_GROUP_SKDM_ONLYmemdb_capt_pkmsg_parse_nack_65_1745544.snapshot-1785024781-pre_pkmsg_REGRESSION_PKMSG_PARSEmemdb_capt_pkmsg_parse_nack_65_1748392.snapshot-1785024800-pre_pkmsg_REGRESSION_PKMSG_PARSEmemdb_capt_session_content_group_ack_74_1745544.snapshot-1785024781-pre_pkmsg_SESSION_CONTENT_GROUPmemdb_capt_session_content_group_ack_74_1748392.snapshot-1785024800-pre_pkmsg_SESSION_CONTENT_GROUPmemdb_capt_skdm_msmsg_no_fallback_ack_78_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_MSMSGmemdb_capt_skdm_msmsg_no_fallback_ack_78_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_MSMSGmemdb_capt_skdm_only_group_ack_77_1745544.snapshot-1785024781-pre_pkmsg_SKDM_ONLY_SESSIONmemdb_capt_skdm_only_group_ack_77_1748392.snapshot-1785024800-pre_pkmsg_SKDM_ONLY_SESSIONmemdb_capt_skdm_plus_skmsg_ack_79_1745544.snapshot-1785024781-pre_pkmsg_SKDM_WITH_SKMSGmemdb_capt_skdm_plus_skmsg_ack_79_1748392.snapshot-1785024800-pre_pkmsg_SKDM_WITH_SKMSGmemdb_capt_status_skdm_only_ack_81_1745544.snapshot-1785024781-pre_pkmsg_STATUS_SKDM_ONLYmemdb_capt_status_skdm_only_ack_81_1748392.snapshot-1785024800-pre_pkmsg_STATUS_SKDM_ONLYmemdb_invalid_spk_id_264_1745544.snapshot-1785024781-pre_pkmsg_INVALID_SPK_ID_MSGmemdb_invalid_spk_id_264_1748392.snapshot-1785024800-pre_pkmsg_INVALID_SPK_ID_MSGmemdb_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789memdb_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789memdb_no_lid_cache_test.snapshot-1785024781-pre_pkmsg_TEST123456789memdb_no_lid_cache_test.snapshot-1785024800-pre_pkmsg_TEST123456789memdb_prod_archive_265_1745544.snapshot-1785024782-pre_pkmsg_memdb_prod_archive_265_1748392.snapshot-1785024801-pre_pkmsg_memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST0memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST1memdb_repeated_msg_test.snapshot-1785024781-pre_pkmsg_TEST2memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST0memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST1memdb_repeated_msg_test.snapshot-1785024800-pre_pkmsg_TEST2src/client/messaging.rs
🛑 Comments failed to post (1)
memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST (1)
68-71: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Stop committing
debug-snapshotsSQLite dumps to the repo. All three of these extensionless binaries are diagnostic output fromPersistenceManager::create_snapshot(src/store/persistence_manager.rs:149-169). They share one root cause and one fix. They embed wall-clock__diesel_schema_migrations.run_onvalues plus SQLite page padding, so they are non-reproducible — the twodup_receiptfiles differ only because they were generated 19 seconds apart — and each one carries rawnoise_key,identity_key, andadv_secret_keyblobs for the test device. A PR whose stated goal is trimming allocator calls per message should not be shipping these.
memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST#L68-L71: remove this artifact from version control and add thememdb_*.snapshot-*pattern to.gitignore.memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST#L68-L71: remove this artifact; it is the 19-second-later twin of the file above and demonstrates the byte-level non-determinism.memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY#L68-L71: remove this artifact under the same ignore rule.If some test genuinely needs to assert against persisted DB state, assert on a queried, normalized projection of the schema and rows rather than on a raw page-level SQLite image.
📍 Affects 3 files
memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST#L68-L71(this comment)memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST#L68-L71memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY#L68-L71🤖 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 `@memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST` around lines 68 - 71, Remove the diagnostic SQLite artifacts memdb_capt_dup_receipt_16_1745544.snapshot-1785024780-pre_pkmsg_EST (lines 68-71), memdb_capt_dup_receipt_16_1748392.snapshot-1785024799-pre_pkmsg_EST (lines 68-71), and memdb_capt_group_skmsg_lock_happy_31_1745544.snapshot-1785024780-pre_pkmsg_GROUP_SKMSG_LOCK_HAPPY (lines 68-71) from version control, then add the memdb_*.snapshot-* pattern to .gitignore. If persisted-state assertions are needed, update them to compare queried, normalized schema and row projections rather than raw SQLite images; PersistenceManager::create_snapshot requires no direct change.Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66bf9ed4c8
ℹ️ 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".
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
This commit accidentally checks in 104 generated test artifacts at the repository root: 52 timestamped 245,760-byte SQLite snapshots and 52 companion JSON files, totaling about 13 MiB. They are not consumed by the source or tests, and future test runs generate differently named copies, so retaining them permanently bloats every clone and source archive; remove these files and ignore the snapshot output pattern.
Useful? React with 👍 / 👎.
| let Some(client) = client.upgrade() else { | ||
| break; | ||
| }; | ||
| if let Err(e) = client.send_ack_for(node.get()).await |
There was a problem hiding this comment.
When reconnect_immediately() runs with deferred ACKs still queued—especially behind a slow transport—it sets expected_disconnect before waiting on outbound_flush. Each queued item then reaches this call, but send_ack_for returns Ok(()) immediately while that flag is set, so the guard is dropped and the flush reports completion without sending the ACK; after reconnect, the server can replay already-processed receipt, notification, call, newsletter, or status stanzas. Drain this queue through a shutdown-safe send path or defer setting the flag until the ACK flush completes.
Useful? React with 👍 / 👎.
66bf9ed to
09fe4b6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d32f66353
ℹ️ 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".
| group_distribution_lock_evictions: group_distribution_locks.evictions, | ||
| group_distribution_lock_eviction_blocks: group_distribution_locks.eviction_blocks, | ||
| resend_rate_limiter_chats: self.resend_rate_limiter.entry_count(), | ||
| transport_ack_queue: self.transport_ack_queue.get().map_or(0, |tx| tx.len()), |
There was a problem hiding this comment.
When the transport stalls, each unbounded slot retains an entire OwnedNodeRef, but this attempted accounting records only Sender::len() as a usize; MemoryReport::total_estimated_bytes() sums only CollectionStats byte fields, so the potentially dominant backlog still contributes zero bytes. The fresh evidence beyond the earlier comment is that the queue is now visible by count while its payload retention remains absent from the report. Report queue retention as CollectionStats or otherwise add estimated payload bytes to the total so leak diagnostics reflect the stalled backlog.
AGENTS.md reference: AGENTS.md:L54-L54
Useful? React with 👍 / 👎.
…esh rng
Naming an outgoing message went through three avoidable costs per send: a
RequestUtils built only to reach the derivation (cloning the unique id, which
the derivation never reads), a Vec staging the digest input, and a StdRng seeded
from the thread generator to produce sixteen bytes, which runs a full ChaCha key
schedule and showed up in the CPU profile.
The derivation is now an associated function fed incrementally, using the
thread-local generator directly. Tests pin the wire shape ("3EB0" plus eighteen
upper-case hex chars), uniqueness inside one second, and the JID-less form.
The drain task exited as soon as the pending map emptied, and under a steady stream that is one spawned task per message: the next capture always arrives after the previous one has been written. Replace it with a single worker woken through a capacity-one channel, so a burst still collapses into one wakeup. The worker is not permanent by ownership. It holds a Weak and the Sender lives in the buffer, so dropping the buffer closes the channel and the worker returns; seal() closes it explicitly because every queue writes inline from that point. Write-behind semantics, the high-water mark and the terminal flush are unchanged. A new test covers captures spread across scheduler turns, which is what a worker that exited early would strand.
Every deferred ack spawned its own task. Feed the persistent-worker pattern already used by delivery receipts instead: a channel slot plus a FlushGuard per ack, with the worker holding a Weak so a dropped client ends it. Two behaviours improve as a side effect. Acks now leave in arrival order, which spawning never guaranteed, and disconnect()'s flush waits for a queued ack because the guard rides the queue until the send returns; previously a spawned ack could still be in flight when the flush completed. A closed scope drops the ack exactly like the spawned path, whose send would have failed on an unavailable transport.
Validating the phash cost a oneshot channel, a spawned task and a ten second timer for every outgoing message, to run a string comparison that almost always succeeds. The waiter map now holds what the check needs, so the read loop compares when the ack lands and spawns only when the server disagrees, which is the path that re-reads caches and can force sender-key redistribution. Ordering is preserved: the waiter is still registered before the stanza goes out, because a fast link can deliver the ack while send_node is still returning. An unanswered ack no longer has a timer to clean up after it, so the keepalive tick sweeps expired phash entries before deciding whether IQs are pending; a leftover waiter would otherwise read as "pending" and silence pings for the life of the connection. The deadline comes from the wall second the send already sampled rather than a fresh read, which the send clock budget test enforces.
The phash rework replaced the only caller I checked, but the VoIP offer also registers an ack waiter and reads the relay out of the response node, and that call site only compiles under the voip feature. Restore the helper and document which of the two registration paths each caller wants: the offer needs the node, a phash check does not and therefore pays no channel per message.
…conds Three review findings on the phash waiter, all real. The sweep ran inside send_keepalive, which the loop skips entirely whenever inbound traffic is recent. A busy connection therefore never swept, and every lost ack left a waiter behind for as long as traffic kept flowing. It now runs on the interval tick, next to the retention cleanup that is placed before the same early return for the same reason. Expiry no longer uses a wall deadline derived from the instant the send started. That instant is sampled well before the waiter is registered, so a slow preparation could register an already-expired waiter and lose the invalidation a mismatching ack must trigger, and the wall clock is explicitly allowed to jump. Waiters now record the sweep epoch, read under the lock the insert already takes, and expire after living through one full sweep: no clock read on the send path, and immune to clock adjustments. The window becomes one keepalive tick (15 to 30 s) rather than a fixed 10 s. Also report the two worker queues in MemoryReport. Both are unbounded and retain inbound nodes plus flush guards, which is exactly the growth the persistent workers introduce, and the report is the documented way to find it. ResponseWaiterEntry.sender is renamed to waiter: half its variants carry no channel now.
Restoring the helper fixed the voip build but broke the default one: the VoIP facade is the sole caller and lives behind voip-runtime, so without that feature the method is dead code and clippy's -D warnings rejects it.
6d32f66 to
1828234
Compare
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 (2)
src/msg_secret_buffer.rs (1)
300-308: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake seal-raced wakeups self-healing
schedule_drain()drops any send error, andseal()closeswake_tx. If it races a producer that just observedsealed == false, the wakeup for that pending entry is lost and the detached worker will not write it. Use an inline flush fallback whentry_send(())fails afterseal(). Also remove the “worker is gone with the buffer” language: a liveseal()closes the channel whileMsgSecretWriteBufferis still alive.🤖 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/msg_secret_buffer.rs` around lines 300 - 308, Update schedule_drain and its caller schedule_or_flush to handle a failed wake-channel try_send after seal(): fall back to an inline flush so pending entries are written even when the worker wakeup is lost. Revise nearby comments to remove any claim that the worker is gone with the buffer, noting that seal() closes the channel while MsgSecretWriteBuffer remains alive.src/send/mod.rs (1)
1694-1730: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister group stanza phash waits in
send_message_impl.
send_group_branchbuilds group stanzas with aphashattribute, but it returnsdm_phash: None, so this branch never reachesregister_phash_waiter. Thread that group phash back throughsend_message_implor the group branch output so group sends can self-correct stale participant/device lists; with the current path, groupdm_phashstays unreachable and we lose the same ack-based stale-list detection that status sends get.🤖 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/send/mod.rs` around lines 1694 - 1730, Update send_group_branch and its SendBranchOutput so the phash generated for group stanzas is returned instead of dm_phash: None. Preserve that value through the group arm in send_message_impl, allowing the existing register_phash_waiter path to register group sends and perform ack-based stale participant/device-list detection.
🤖 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 `@src/msg_secret_buffer.rs`:
- Around line 300-308: Update schedule_drain and its caller schedule_or_flush to
handle a failed wake-channel try_send after seal(): fall back to an inline flush
so pending entries are written even when the worker wakeup is lost. Revise
nearby comments to remove any claim that the worker is gone with the buffer,
noting that seal() closes the channel while MsgSecretWriteBuffer remains alive.
In `@src/send/mod.rs`:
- Around line 1694-1730: Update send_group_branch and its SendBranchOutput so
the phash generated for group stanzas is returned instead of dm_phash: None.
Preserve that value through the group arm in send_message_impl, allowing the
existing register_phash_waiter path to register group sends and perform
ack-based stale participant/device-list detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce5ca7c9-3801-446f-9690-896be26b1414
📒 Files selected for processing (13)
.gitignoresrc/client.rssrc/client/accessors.rssrc/client/lifecycle.rssrc/client/messaging.rssrc/client/node_io.rssrc/client/tests.rssrc/keepalive.rssrc/msg_secret_buffer.rssrc/request.rssrc/send/mod.rssrc/test_utils.rswacore/src/request.rs
1 participant
Four independent cuts to the per-message cost of a DM send/receive round trip, found by profiling the harness
pingpongscenario (120k messages at 12k/s) with the rss, dhat, cpu and tokio passes and then re-checking each conclusion against the code.The theme is per-message task and channel plumbing. A steady-state round trip was spawning six tokio tasks; four of them existed for work that either does not need a task at all or can be served by a worker that already exists.
What changes
1. The message id is derived without a staging buffer or a fresh RNG. Naming a message built a
RequestUtilsonly to reach the derivation, which cloned the unique id that the derivation never reads, staged the digest input in aVec, and seeded aStdRngfrom the thread generator to get sixteen bytes, running a full ChaCha key schedule per message (visible in the CPU profile aschacha20::rng_inner). It is now an associated function fed incrementally from the thread-local generator. Tests pin the wire shape (3EB0plus eighteen upper-case hex chars), uniqueness inside one second, and the JID-less form.2. Message secrets drain from one worker instead of a task per capture. The drain task exited as soon as the pending map emptied, which under a steady stream is one spawned task per message: the next capture always arrives after the previous one has been written. A single worker woken through a capacity-one channel keeps the coalescing behaviour. The worker is not permanent by ownership: it holds a
Weak, and theSenderlives in the buffer, so dropping the buffer closes the channel and ends the worker;seal()closes it explicitly because every queue writes inline from that point.3. Deferred acks go through one worker instead of a task per ack. This reuses the pattern already in place for delivery receipts. Two behaviours improve as a side effect: acks now leave in arrival order, which spawning never guaranteed, and
disconnect()'s flush waits for a queued ack because theFlushGuardrides the queue until the send returns.4. The ack phash is compared inline. Validating it cost a oneshot channel, a spawned task and a ten second timer per outgoing message, to run a string comparison that almost always succeeds. The waiter map now carries what the check needs, so the read loop compares when the ack lands and spawns only on a mismatch, which is the path that re-reads caches and can force sender-key redistribution.
Two details there are load-bearing. The waiter is still registered before the stanza goes out, because a fast link can deliver the ack while
send_nodeis still returning. And with no timer to clean up after an unanswered ack, the keepalive tick now sweeps expired phash entries before deciding whether IQs are pending: a leftover waiter would otherwise read as "pending" and silence pings for the life of the connection. The deadline reuses the wall second the send already sampled instead of reading the clock again, which the existing send clock-budget test enforces.Measurements
Harness
pingpong, 120k messages at 12k/s, againstb08ba338. Runs are interleaved ABBA/BAAB rather than blocked, so host drift is not attributed to the branch. Every run in every table hadlost=0andack=120000.MODE=normalThe CPU figure is the one worth qualifying. Welch t = 2.47, df = 8, p ~ 0.013 two-sided, so it survives the noise across five interleaved pairs, but the min/max ranges do touch (main 10.17-11.00, branch 9.54-10.35). Treat it as a real but modest effect, not a headline. The allocation figures have no such ambiguity: the standard deviations are two orders of magnitude below the difference.
Note that
allocator callscountsalloc/alloc_zeroed/realloccalls, not live objects, andbytes requestedcounts new allocations plus the positive delta of arealloc. They measure pressure on the allocator, not heap residency.Call-site proof
Aggregate numbers do not show what was removed, so the dhat pass (20k messages) is diffed per call site. Everything the change targets disappears, and the only new cost is the listener the persistent workers park on:
Vec<u8>::with_capacity_in(message-id staging buffer among others)Box<TokioRuntime::spawn::{closure}>Box<tokio::runtime::task::Cell>(the spawned tasks themselves)Box<bytes::Shared>Box<tokio::time::sleep::Sleep>(the phash timer)Box<maybe_deferred_ack::{closure}>Box<ArcInner<oneshot::Inner<OwnedNodeRef>>>(the phash oneshot)Box<spawn_phash_validation::{closure}>Box<ArcInner<OwnedNodeRef>>Box<MsgSecretWriteBuffer::schedule_drain::{closure}>Box<event_listener::InnerListener>(workers parked on their channel)Verification
cargo fmt --all,cargo clippy --all-targetsclean,cargo test -p whatsapp-rust --libgreen (1183 tests, including three new ones: the message-id shape/uniqueness pins and a sweep test asserting that expired phash waiters are dropped while IQ waiters are never touched).What is deliberately not here
Two further candidates were identified and left out of this PR rather than rushed into it:
memcpy). It changes the encrypt/framing path, and the sender should first be made to poison itself on a transport error: today the write counter advances only aftertransport.sendreturnsOk, so an ambiguous partial write can let the same counter, and therefore the same AES-GCM nonce, be reused. That fix belongs in front of the optimization, in its own PR.Review notes
Two findings on the ack worker were checked against the code and are recorded here rather than changed:
Acks skipped during an expected disconnect.
send_ack_forreturnsOk(())early whileexpected_disconnectis set, so a queued ack is dropped rather than sent. That early return predates this PR and the previous spawn-per-ack path hit it identically, so no ack is lost that was being sent before. What is new is that theFlushGuardmakesdisconnect()'s flush wait for entries that will then be skipped, which costs a short wait rather than losing anything. Sending pending acks before marking the disconnect expected is a change to the shutdown ordering and belongs in its own PR.Byte accounting for the queues.
MemoryReportreports both worker queues by entry count, not by retained bytes, so a backlog contributes zero tototal_estimated_bytes(). Anasync_channelcannot be iterated, so the only way to total the retained nodes is a counter maintained on every push and pop, which puts bookkeeping on the hot path for a secondary diagnostic. The entry count already surfaces the backlog, and it is what the pre-existing delivery-receipt queue reports too.