perf(core): reduce history sync and decrypt allocation churn - #1058
Conversation
jlucaso1
added
api-design
breaking-change
performance
size-increase-ok
ChatGPT Codex Connector
📝 WalkthroughWalkthroughHistory-sync processing now tracks retained payloads and lifetime peaks, extracts secrets through streaming sinks, and persists fixed-size entries. Session decryption supports owned retryable ciphertext. Crypto, storage, decompression, and dispatch paths reduce intermediate allocations. ChangesHistory sync and memory accounting
Owned decryption and crypto
Storage and dispatch
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HistorySyncActivity
participant HistorySyncParser
participant MsgSecretStore
Client->>HistorySyncActivity: track retained payload
Client->>HistorySyncParser: process compressed bytes with sink
HistorySyncParser->>Client: collected secret entries
Client->>MsgSecretStore: persist secret entries
HistorySyncActivity-->>Client: update task and payload peaks
Possibly related PRs
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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Merging this PR will improve performance by ×2.1
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
|
| Filename | Overview |
|---|---|
| src/sync_task.rs | New file: generation-bound RAII HistorySyncActivity and HistorySyncTaskTracker with correct mutex/event-listener ordering, stale-task isolation, and two targeted unit tests. |
| src/msg_secret_buffer.rs | Adds backpressure high-water mark with event-listener synchronisation; switches to hashbrown KeyRef/Equivalent for zero-copy lookups; replaces quadratic finish_batch scan with O(n log n) sorted pointer search; capacity_available.listen() registered inside pending mutex avoids missed-notification races. |
| wacore/libsignal/src/protocol/session_cipher.rs | OwnedCiphertextMessage and SignalDecryptInput unify borrowed/owned decrypt paths; MAC-before-consume ordering is preserved; !is_available() guards in both current-session and archived-session branches prevent silent fallthrough after the wire body is consumed; retry semantics unchanged for borrowed path. |
| wacore/src/history_sync.rs | HistoryMsgSecretRecordVisitor/Sink abstraction eliminates intermediate owned-record allocation for the collector path; deferred key/participant parsing skips UTF-8 validation for the common no-secret message; RECORD_RESERVE_BYTE_CAP replaces record-count cap for correct per-item-size budgeting. |
| src/history_sync.rs | HistorySecretSeedCollector builds MsgSecretEntry rows directly with shared Arc identifiers; sender-slot overflow bug fixed by returning before the bot-chat alias branch for from_me=true; new test covers outgoing-bot and incoming-bot alias counts explicitly. |
| wacore/src/store/traits.rs | MessageSecret type alias makes the fixed 32-byte size unrepresentable incorrectly; MsgSecretEntry identifiers changed to Arc; put_msg_secret signature tightened to &[u8; MESSAGE_SECRET_SIZE]. |
| storages/sqlite-storage/src/sqlite_store.rs | Arc::new(entries) avoids double allocation versus Arc::from; per-chunk record materialisation stays within one atomic transaction; MSG_SECRET_INSERT_CHUNK_SIZE constant matches the 8-column comment; test extended to cross the chunk boundary and verify merged-order correctness. |
| wacore/binary/src/zlib_pool.rs | pump() compacts the consumed prefix on every call instead of conditionally; reserve is now lazy (only when spare capacity is exhausted); RETAINED_CAPACITY halved to one CHUNK window; new test asserts sub-window records don't grow the buffer beyond CHUNK. |
| wacore/libsignal/src/crypto/provider.rs | aes_256_cbc_decrypt_in_place and hmac_sha256_two_part added to SignalCryptoProvider with correct default fallbacks for external providers; RustCryptoProvider delegates aes_256_cbc_decrypt to in_place to avoid holding two full-size buffers simultaneously. |
| wacore/noise/src/framing.rs | mem::take when the frame exactly fills the buffer preserves unique ownership for downstream in-place decryption; split_to fallback unchanged for multi-frame buffers; test asserts freeze().is_unique() for the drained-buffer case. |
| wacore/src/store/in_memory.rs | MsgSecretKey/MsgSecretKeyRef with hashbrown Equivalent enables zero-copy lookups; initial-batch reserve heuristic avoids over-growing on predominantly-update subsequent batches; get_msg_secret returns secret.to_vec() from the fixed-size MessageSecret array. |
Reviews (8): Last reviewed commit: "refactor(storage): clarify message secre..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/node_io.rs`:
- Line 534: Remove the redundant comment immediately before the deferred ACK
logic in the node I/O flow, leaving the underlying ACK handling unchanged.
- Around line 509-511: Update the "receipt" arm to pass node directly to
handle_receipt_inline instead of cloning the Arc, preserving ownership transfer
since node is not used afterward.
In `@src/msg_secret_buffer.rs`:
- Around line 133-134: Redesign the pending lookup in src/msg_secret_buffer.rs
lines 133-134 and the msg_secrets lookup in wacore/src/store/in_memory.rs lines
914-915 to query with (&str, &str, &str) without constructing Arc values on
reads. Use a zero-allocation heterogeneous lookup mechanism such as
hashbrown::HashMap with Equivalent, while preserving the existing returned
secret and timestamp behavior.
In `@wacore/src/store/traits.rs`:
- Around line 18-22: Change the MessageSecret type alias to the inline
fixed-size byte array rather than a boxed array, and update all MsgSecretEntry
construction, persistence, and test call sites to remove Box::new while
preserving the existing MESSAGE_SECRET_SIZE length.
🪄 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: 42f08cb0-33f9-45ce-9dd4-33744f6dbef0
📒 Files selected for processing (29)
agent_docs/observability.mdsrc/client.rssrc/client/accessors.rssrc/client/app_state.rssrc/client/lifecycle.rssrc/client/node_io.rssrc/client/sessions.rssrc/history_sync.rssrc/message.rssrc/message/msg_secret.rssrc/message/receive.rssrc/message/tests.rssrc/msg_secret_buffer.rssrc/send/mod.rssrc/sync_task.rsstorages/sqlite-storage/src/sqlite_store.rswacore/benches/history_sync_benchmark.rswacore/binary/src/zlib_pool.rswacore/libsignal/src/crypto/aes_cbc.rswacore/libsignal/src/crypto/mod.rswacore/libsignal/src/crypto/provider.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/session_cipher.rswacore/noise/src/framing.rswacore/src/download.rswacore/src/history_sync.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
|
Review follow-up in bb00cf4:
Validation is green across formatting, clippy with all features and targets, workspace tests, doctests, SQLite storage tests, and the complete E2E suite. Focused interleaved A/B measurements also confirmed lower allocation volume and faster construction, cloning, insertion, and lookup. |
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 (2)
src/client/node_io.rs (1)
521-525: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass
nodeby value to avoid an unnecessaryArcclone.Look, we're optimizing the
messageandreceiptarms to pass ownership ofnodeand skip the reference count bump. That's exactly the kind of relentless focus on efficiency we need. But you missed it right here in the fallback_arm! Since this is the last use ofnodein this branch, just pass it by value. We need to be ruthless about performance on these hot paths; don't leave easy wins on the table.⚡ Proposed fix
_ => { let handled = self .stanza_router - .dispatch(self.clone(), Arc::clone(&node), &mut cancelled) + .dispatch(self.clone(), node, &mut cancelled) .await;🤖 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/node_io.rs` around lines 521 - 525, Update the fallback dispatch call in the node-handling branch to pass node by value instead of cloning its Arc, while preserving the existing cancellation and handled-result flow.src/history_sync.rs (1)
375-392: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRemove the redundant detached async task.
Look, we're building WhatsApp, and this kind of code doesn't scale. What on earth is this async choreography? You're allocating a oneshot channel, boxing a closure, spawning an entirely separate async task, detaching it, and awaiting the channel... just to wait for a blocking operation that you could have awaited directly!
Even if your custom
spawn_blockingsignature forces you to use a channel to extract the return value, you should still just.awaitthe future it returns right here in the current task. Spawning a detached task to do nothing but.awaitit wastes allocations and runtime overhead. Let's keep things efficient.⚙️ Move fast and fix this
- } else { - let (result_tx, result_rx) = futures::channel::oneshot::channel(); - let blocking_fut = self.runtime.spawn_blocking(Box::new(move || { - let result = process_history_sync_bytes_with_record_sink( - compressed_data, - own_user.as_deref(), - true, - &mut secret_collector, - ); - let _ = result_tx.send((result, secret_collector.into_entries())); - })); - self.runtime - .spawn(Box::pin(async move { - blocking_fut.await; - })) - .detach(); - result_rx.await.ok() - }; + } else { + let (result_tx, result_rx) = futures::channel::oneshot::channel(); + let blocking_fut = self.runtime.spawn_blocking(Box::new(move || { + let result = process_history_sync_bytes_with_record_sink( + compressed_data, + own_user.as_deref(), + true, + &mut secret_collector, + ); + let _ = result_tx.send((result, secret_collector.into_entries())); + })); + let _ = blocking_fut.await; + result_rx.await.ok() + };🤖 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/history_sync.rs` around lines 375 - 392, Remove the intermediate detached task around spawn_blocking in the history-sync branch. Await the blocking_fut returned by self.runtime.spawn_blocking directly in the current async flow, while preserving the existing result_tx/result_rx channel handling and result_rx.await.ok() 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/msg_secret_buffer.rs`:
- Around line 52-55: Implement bounded backpressure in
MsgSecretWriteBuffer::queue: after adding an entry to pending, detect when the
map reaches a defined high-water mark and make the producer await the in-flight
flush before accepting more work. Reuse the existing drain/flush coordination,
preserving detached draining below the threshold and ensuring concurrent
producers cannot bypass the bound.
---
Outside diff comments:
In `@src/client/node_io.rs`:
- Around line 521-525: Update the fallback dispatch call in the node-handling
branch to pass node by value instead of cloning its Arc, while preserving the
existing cancellation and handled-result flow.
In `@src/history_sync.rs`:
- Around line 375-392: Remove the intermediate detached task around
spawn_blocking in the history-sync branch. Await the blocking_fut returned by
self.runtime.spawn_blocking directly in the current async flow, while preserving
the existing result_tx/result_rx channel handling and result_rx.await.ok()
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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4304015c-73dd-47d9-9800-9bcce4a85b8c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlsrc/client/node_io.rssrc/client/tests.rssrc/features/chat_actions.rssrc/history_sync.rssrc/lib.rssrc/message/msg_secret.rssrc/message/tests.rssrc/msg_secret_buffer.rssrc/send/mod.rsstorages/sqlite-storage/src/sqlite_store.rswacore/Cargo.tomlwacore/src/store/in_memory.rswacore/src/store/traits.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: 2ad92a5dfe
ℹ️ 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: 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/history_sync.rs`:
- Around line 751-756: Extend the assertions in the history-sync test after
collector.into_entries() to verify that entries[0].sender matches
entries[0].chat, mirroring the existing entries[1] sender assertion while
preserving all other checks.
In `@wacore/libsignal/benches/libsignal_benchmark.rs`:
- Around line 680-683: Update the benchmark conversion around
SignalMessage::try_from to pass ownership of ciphertext_bytes directly instead
of calling as_slice(), so the timed path measures the intended zero-copy owned
decryption behavior without constructing a slice-backed Bytes value.
🪄 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: 67579e11-e4e2-46de-baf6-295f4621cf5d
📒 Files selected for processing (3)
src/history_sync.rswacore/libsignal/benches/libsignal_benchmark.rswacore/libsignal/src/protocol/session_cipher.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 951bda7f22
ℹ️ 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 (2)
src/history_sync.rs (2)
172-179: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftReuse the msg-secret write-behind here.
HistorySecretSeedCollectorstill keeps every retainedMsgSecretEntryin oneVecuntil parsing finishes, and this path writes the whole batch straight toput_msg_secrets(...)instead of the boundedmsg_secret_bufferused by live captures. Large history syncs can still spike memory by the full retained set; feed entries into the existing buffer in bounded chunks, or cap the collector before merge.🤖 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/history_sync.rs` around lines 172 - 179, The history sync path in HistorySecretSeedCollector currently accumulates all MsgSecretEntry values in a Vec and writes them in one batch. Reuse the existing msg_secret_buffer write-behind mechanism when adding entries, flushing bounded chunks to put_msg_secrets(...) instead of retaining the full set; preserve the existing merge behavior and ensure any remaining buffered entries are flushed when collection completes.
240-241: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winTrack the inline payload’s retained buffer, not just its slice length. The inline path uses
Bytes::len(), while the downloaded path usesVec::capacity(). Since inline payloads share the decrypt buffer, this undercounts retained memory for inline blobs and skews the peak metric; use a consistent retained-allocation estimate or document that this counter is payload length only.🤖 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/history_sync.rs` around lines 240 - 241, Update the retained-size calculation before begin_history_sync_task to account for the inline payload’s retained backing allocation rather than only Bytes::len(), matching the downloaded path’s Vec::capacity() estimate. Use the appropriate Bytes capacity/allocation API available in this codebase, or explicitly redefine/document the counter as payload length if allocation cannot be measured consistently.
🤖 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/history_sync.rs`:
- Around line 172-179: The history sync path in HistorySecretSeedCollector
currently accumulates all MsgSecretEntry values in a Vec and writes them in one
batch. Reuse the existing msg_secret_buffer write-behind mechanism when adding
entries, flushing bounded chunks to put_msg_secrets(...) instead of retaining
the full set; preserve the existing merge behavior and ensure any remaining
buffered entries are flushed when collection completes.
- Around line 240-241: Update the retained-size calculation before
begin_history_sync_task to account for the inline payload’s retained backing
allocation rather than only Bytes::len(), matching the downloaded path’s
Vec::capacity() estimate. Use the appropriate Bytes capacity/allocation API
available in this codebase, or explicitly redefine/document the counter as
payload length if allocation cannot be measured consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5befe114-1a65-43b6-8d81-d29ee5f75cf1
📒 Files selected for processing (2)
src/history_sync.rswacore/libsignal/benches/libsignal_benchmark.rs
|
Follow-up on the two outside-diff findings from review 4736696561:
The full workspace tests, all-target Clippy, formatting, focused SQLite/history-accounting regressions, and the wasm32 release build pass locally. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4197af7549
ℹ️ 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
🤖 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 `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 3382-3387: Rename the inner loop variable in the chunk-processing
loop from entries to chunk, and update its references when building records.
Preserve the surrounding batching logic and the outer entries collection
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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f566bd5-3e52-4524-a31b-1547995c59d3
📒 Files selected for processing (8)
src/client.rssrc/client/accessors.rssrc/client/app_state.rssrc/client/sessions.rssrc/history_sync.rssrc/sync_task.rsstorages/sqlite-storage/src/sqlite_store.rswacore/libsignal/benches/libsignal_benchmark.rs
1 participant
Why
History-sync ingestion, persistence, and decryption still performed avoidable intermediate allocations and full-buffer copies on high-volume paths. Those costs increased allocator churn and peak memory during initial syncs, while the memory report did not attribute in-flight history payloads precisely enough.
What changed
API notes
MsgSecretEntryidentifiers now useArc<str>, and its secret uses the fixed-sizeMessageSecretrepresentationput_msg_secretaccepts a fixedMessageSecretMeasured impact
.text, and +12.17 KiB allocated versusmain; the local release A/B recovered 28.2 KiB.textversus the earlier PR head, and the final artifact remains within budgetValidation
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test --workspace --exclude e2e-testsRUSTFLAGS='--cfg getrandom_backend="wasm_js"' cargo build -p whatsapp-rust --lib --release --target wasm32-unknown-unknown --no-default-features